Improve xlsx import #1271
Improve xlsx import #1271
Conversation
alex-Arc
force-pushed
the
imporove-xlsx-import
branch
from
6842841
to
319f6e6
Compare
apps/client/src/features/app-settings/panel/sources-panel/import-map/importMapUtils.ts
Outdated
Show resolved
Hide resolved
apps/client/src/features/app-settings/panel/sources-panel/import-map/importMapUtils.ts
Outdated
Show resolved
Hide resolved
alex-Arc
force-pushed
the
imporove-xlsx-import
branch
from
319f6e6
to
6580ee5
Compare
alex-Arc
force-pushed
the
imporove-xlsx-import
branch
from
6580ee5
to
cf48825
Compare
apps/client/src/features/app-settings/panel/sources-panel/import-map/importMapUtils.ts
Outdated
Show resolved
Hide resolved
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis pull request introduces several changes across multiple components and services, primarily focusing on enhancing the handling of custom fields and improving input validation. The Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (5)
apps/client/src/features/app-settings/panel/sources-panel/preview/PreviewRundown.tsx (1)
Line range hint
106-111: Suggestion for custom field value renderingThe use of
fieldKeysfor rendering custom field values is correct and consistent with earlier changes. The existence check before accessingevent.custom[field]is also a good practice. However, consider the following improvements:
- Make the empty string fallback more explicit.
- Use optional chaining and nullish coalescing for a more concise implementation.
Here's a suggested refactor:
fieldKeys.map((field) => { - let value = ''; - if (field in event.custom) { - value = event.custom[field]; - } - return <td key={field}>{value}</td>; + const value = event.custom?.[field] ?? ''; + return <td key={field}>{value}</td>; })}This change maintains the same functionality while making the code more concise and explicit about the fallback value.
apps/client/src/features/app-settings/panel/sources-panel/import-map/ImportMapForm.tsx (1)
192-195: LGTM: Updated validation to allow spaces in custom field namesThe validation logic has been successfully updated to allow spaces in custom field names, which aligns with the PR objectives. The implementation using
isAlphanumericWithSpaceis correct and the error message is clear.Consider adding a check for empty strings to improve user experience:
validate: (value) => { + if (!value.trim()) return 'Field name cannot be empty'; if (!isAlphanumericWithSpace(value)) return 'Only alphanumeric characters and space are allowed'; return true; },This addition will prevent users from submitting empty field names while still allowing spaces.
apps/server/src/utils/parser.ts (2)
58-76: Improved custom field handling with color preservation.The changes to
getCustomFieldDatafunction enhance the handling of custom fields by preserving existing color information. This aligns well with the PR objective of improving custom field management.Consider using optional chaining for a more concise color assignment:
-const colour = ontimeKey in existingCustomFields ? existingCustomFields[ontimeKey].colour : ''; +const colour = existingCustomFields[ontimeKey]?.colour ?? '';This change would slightly improve readability and reduce the need for an explicit check.
Line range hint
87-101: Enhanced Excel parsing with existing custom fields support.The changes to
parseExcelfunction improve the handling of custom fields during Excel import by incorporating existing custom field data. This enhancement aligns well with the PR objective of improving Excel import functionality.Consider adding a type annotation for the
existingCustomFieldsparameter to improve code readability and maintainability:export const parseExcel = ( excelData: unknown[][], - existingCustomFields: CustomFields, + existingCustomFields: CustomFields, options?: Partial<ImportMap>, ): ExcelData => { // ... (rest of the function)This change would make the function signature more explicit about the expected type of
existingCustomFields.apps/server/src/utils/__tests__/parser.test.ts (1)
978-978: Inconsistent casing in custom field keys.The key
'User1'uses mixed casing, while other keys like'user0'are in lowercase. This might lead to issues due to case sensitivity.Consider standardizing the casing of custom field keys to avoid potential inconsistencies or bugs related to case-sensitive operations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
apps/server/package.jsonis excluded by none and included by nonepnpm-lock.yamlis excluded by!**/pnpm-lock.yamland included by none
📒 Files selected for processing (6)
- apps/client/src/features/app-settings/panel/sources-panel/import-map/ImportMapForm.tsx (2 hunks)
- apps/client/src/features/app-settings/panel/sources-panel/preview/PreviewRundown.tsx (3 hunks)
- apps/server/src/api-data/excel/excel.service.ts (2 hunks)
- apps/server/src/services/sheet-service/SheetService.ts (3 hunks)
- apps/server/src/utils/tests/parser.test.ts (14 hunks)
- apps/server/src/utils/parser.ts (4 hunks)
🧰 Additional context used
🔇 Additional comments (21)
apps/client/src/features/app-settings/panel/sources-panel/preview/PreviewRundown.tsx (2)
27-28: Improved handling of custom fieldsThe introduction of
fieldKeysandfieldLabelsis a good improvement. This separation of concerns allows for more flexible usage of custom field data throughout the component.
48-49: Correct usage of fieldLabels for table headersThe update to use
fieldLabelsfor rendering table headers is correct and consistent with the earlier changes. This ensures that user-friendly labels are displayed in the table.apps/client/src/features/app-settings/panel/sources-panel/import-map/ImportMapForm.tsx (2)
6-6: LGTM: Updated import to allow spaces in custom fieldsThe import of
isAlphanumericWithSpacefrom 'ontime-utils' aligns with the PR objective of allowing spaces in custom imports. This change sets the foundation for the updated validation logic.
Line range hint
1-238: Overall assessment: Changes successfully implement PR objectivesThe modifications to
ImportMapForm.tsxeffectively implement the PR objective of allowing spaces in custom imports. The changes are focused and minimal, which reduces the risk of unintended side effects. The updated import and validation logic work together to provide a more flexible user experience when defining custom field names.To ensure that these changes don't affect other parts of the codebase, please run the following verification script:
This will help identify any other locations where
isAlphanumericis used, which might need to be updated toisAlphanumericWithSpacefor consistency.✅ Verification successful
To address the issue with the previous shell script, please run the following corrected verification script:
To resolve the issue with the previous shell script, please run the following corrected verification script:
Overall assessment: Changes successfully implement PR objectives
The modifications to
ImportMapForm.tsxeffectively implement the PR objective of allowing spaces in custom imports. The changes are focused and minimal, which reduces the risk of unintended side effects. The updated import and validation logic are consistent with existing usages ofisAlphanumericWithSpaceacross the codebase, ensuring a seamless and flexible user experience when defining custom field names.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any other usages of 'isAlphanumeric' that might need updating # Test: Search for other usages of 'isAlphanumeric' rg "isAlphanumeric" --type ts --type tsxLength of output: 68
Script:
# #!/bin/bash # Description: Check for any other usages of 'isAlphanumeric' that might need updating # Corrected Test: Search for other usages of 'isAlphanumeric' in .ts and .tsx files rg "isAlphanumericWithSpace" --type ts --type tsxLength of output: 77
Script:
# #!/bin/bash # Description: Check for any other usages of 'isAlphanumericWithSpace' that might need updating # Corrected Test: Search for other usages of 'isAlphanumericWithSpace' in .ts and .tsx files using glob patterns rg "isAlphanumericWithSpace" -g "*.ts" -g "*.tsx"Length of output: 1613
apps/server/src/services/sheet-service/SheetService.ts (4)
21-21: LGTM: New import for custom fields handling.The addition of
getCustomFieldsimport aligns with the PR objective of enhancing custom field support. This function will be used to retrieve custom fields for Excel parsing, improving the flexibility of the import process.
292-292: LGTM: Improved Excel parsing with custom fields support.The modification to include
getCustomFields()in theparseExcelfunction call enhances the upload process by incorporating custom fields. This change ensures that the Excel parsing takes into account user-defined fields, aligning with the PR's goal of improving custom import handling.
369-369: LGTM: Consistent custom fields handling in download operation.The addition of
getCustomFields()to theparseExcelfunction call in the download process mirrors the change made in the upload function. This ensures consistent handling of custom fields across both operations, maintaining data integrity and improving the overall robustness of the Excel import/export functionality.
21-21: Summary: Improved custom fields handling in Sheet Service.The changes made to this file consistently enhance the handling of custom fields in both upload and download operations. The addition of
getCustomFields()to the Excel parsing process ensures that user-defined fields are properly incorporated, aligning well with the PR's objective of improving xlsx import functionality. These modifications contribute to a more flexible and robust integration with Google Sheets.Also applies to: 292-292, 369-369
apps/server/src/utils/parser.ts (1)
Line range hint
1-451: Overall improvements in custom field handling and Excel import.The changes in this file successfully enhance the custom field handling and Excel import functionality. The modifications to
getCustomFieldDataandparseExcelfunctions are well-aligned with the PR objectives and improve the overall robustness of the codebase.The changes are focused and effective, maintaining the existing structure while adding necessary functionality. Good job on improving the Excel import process and custom field management!
apps/server/src/utils/__tests__/parser.test.ts (12)
5-5: ImportingCustomFieldsis appropriate.The
CustomFieldstype is now imported from'ontime-types', which is necessary for the type definitions used in the test cases below.
Line range hint
792-817: Test case correctly verifiesgetCustomFieldDataoutput.The test ensures that
getCustomFieldDatagenerates the correctcustomFieldsandcustomFieldImportKeyswhen provided with animportMapand an empty existing custom fields object.
818-865: Verify consistency of custom field labels.In the existing
customFields, the label for'lighting'is'lx', but after processing, the label becomes'lighting'. This changes the label from'lx'to'lighting', which may not be intended.Please confirm if the transformation of the label from
'lx'to'lighting'in the result is expected. If the original labels should be preserved, consider adjusting the implementation to maintain consistency.
1011-1031: Test accurately checks retention of color information in custom fields.The test verifies that when parsing Excel data, the existing custom fields' color information is retained. The assertions confirm that colors for
'user0','User1', and'user2'are correctly preserved.
1188-1188: EnsureparseExcelhandles absence of custom fields gracefully.The test checks parsing Excel data without custom fields specified. Confirm that the implementation can handle cases where custom fields are not provided without errors.
Please verify that the
parseExcelfunction appropriately manages scenarios with no custom fields to prevent unexpected behavior.
1294-1294: Verify event type handling inparseExcel.The test ensures that unknown event types are correctly ignored during parsing. This is important for robustness, but make sure that valid events are not mistakenly skipped.
Consider adding additional validation or logging to capture any events that are ignored to aid in debugging if valid events are missed.
1387-1387: Correct handling of block events in Excel import.The test confirms that block events are imported correctly from Excel data. This maintains the integrity of the event sequence in the rundown.
1457-1457: Ensure default timer type is set when missing or whitespace.The test verifies that when the 'timer type' column is empty or contains only whitespace, events default to
TimerType.CountDown.
1575-1575: Accurate parsing of timer types with leading/trailing whitespace.The test checks that timer types with leading or trailing whitespace are correctly interpreted, such as
' count-up '. This ensures that user input variations do not cause parsing errors.
1616-1616: Time conversions are correctly handled for AM/PM formats.The test confirms that times specified in AM/PM formats are accurately converted to 24-hour timestamps. This is essential for correct event scheduling.
1653-1653: Handle leading and trailing whitespace in headers and data.The test ensures that both import map keys and Excel data with leading or trailing whitespace are parsed correctly. This improves the robustness of the import functionality.
1705-1705: Verify link start logic in event parsing.The test checks the correct application of the
linkStartproperty across various events, including those following blocks. It's important to ensure that linked events have accurate start times.Please ensure that events with
linkStartset totruecorrectly adjust theirtimeStartbased on the preceding event'stimeEnd, even when blocks are present.
node-xlsxtoxlsx