[CUS-9450] added 2 new nlps for supporting sheet names. #276
Conversation
WalkthroughMaven version bumped to 1.0.17 and two new Java WebAction classes were added to count non-empty Excel cells/rows/columns from local paths or remote URLs, with sheet selection, input validation, logging, and error handling. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
excelActions_cloud/src/main/java/com/testsigma/addons/web/StoreNonEmptyCellCountForColumnOrRow.java (1)
255-273: Temporary file is never cleaned up after use.The temp file created at line 260 persists after execution. Consider deleting it in a
finallyblock or marking it withdeleteOnExit(). Additionally,copyURLToFilehas no timeout, which could cause the action to hang on slow/unresponsive servers.Consider using the overload with timeout:
- FileUtils.copyURLToFile(urlObject, tempFile); + FileUtils.copyURLToFile(urlObject, tempFile, 30000, 60000); // 30s connect, 60s read timeout + tempFile.deleteOnExit();excelActions_cloud/src/main/java/com/testsigma/addons/web/StoreNonEmptyCellCountWithSheet.java (2)
84-109: Consider extracting shared utility methods.
getSheetByNameOrIndexanddownloadFileFromUrlare duplicated between this class andStoreNonEmptyCellCountForColumnOrRow. Consider extracting these to a shared utility class (e.g.,ExcelFileUtils) to reduce duplication and centralize maintenance.The
downloadFileFromUrlmethod also has the same temp file leak and missing timeout issues mentioned in the other class.Also applies to: 171-189
111-123: Silent fallback to 0 for unrecognized count types.If
countTypedoesn't match any expected value, the method returns 0 without logging. While the@TestDataannotation restricts input, consider adding anelseclause with a warning log for defensive programming.} else if (countType.equalsIgnoreCase("columns")) { count = countMaxColumns(sheet); + } else { + // This shouldn't happen due to allowedValues constraint, but log for safety + throw new IllegalArgumentException("Unknown count type: " + countType); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
excelActions_cloud/pom.xml(1 hunks)excelActions_cloud/src/main/java/com/testsigma/addons/web/StoreNonEmptyCellCountForColumnOrRow.java(1 hunks)excelActions_cloud/src/main/java/com/testsigma/addons/web/StoreNonEmptyCellCountWithSheet.java(1 hunks)
🔇 Additional comments (4)
excelActions_cloud/pom.xml (1)
9-9: LGTM!Version bump to 1.0.17 is appropriate for the new Excel action classes being added.
excelActions_cloud/src/main/java/com/testsigma/addons/web/StoreNonEmptyCellCountForColumnOrRow.java (2)
117-155: LGTM!The column resolution logic correctly handles multiple input formats (numeric index, letter notation, header name) with appropriate fallback and logging.
186-211: LGTM!Sheet resolution handles both numeric indices (1-based for users) and names correctly with proper bounds checking.
excelActions_cloud/src/main/java/com/testsigma/addons/web/StoreNonEmptyCellCountWithSheet.java (1)
155-169: LGTM!The
countMaxColumnsmethod correctly iterates over all cells and tracks the rightmost non-empty column index.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
excelActions_cloud/src/main/java/com/testsigma/addons/web/StoreNonEmptyCellCountWithSheet.java (1)
112-124: Unhandled count type returns 0 silently.If an unexpected
countTypeis passed, the method returns 0 without any warning. This could mask configuration errors.public static int calculateExcelDataCount(String countType, XSSFSheet sheet) { - int count = 0; - if (countType.equalsIgnoreCase("non-empty cells")) { - count = countNonEmptyCells(sheet); + return countNonEmptyCells(sheet); } else if (countType.equalsIgnoreCase("rows")) { - count = countNonEmptyRows(sheet); + return countNonEmptyRows(sheet); } else if (countType.equalsIgnoreCase("columns")) { - count = countMaxColumns(sheet); + return countMaxColumns(sheet); + } else { + throw new IllegalArgumentException("Unknown count type: " + countType); } - - return count; }excelActions_cloud/src/main/java/com/testsigma/addons/web/StoreNonEmptyCellCountForColumnOrRow.java (1)
186-211: Significant code duplication with StoreNonEmptyCellCountWithSheet.
getSheetByNameOrIndex()anddownloadFileFromUrl()are identical in both classes. Consider extracting these into a shared utility class.Create a utility class like
ExcelActionUtils:public final class ExcelActionUtils { private ExcelActionUtils() {} public static XSSFSheet getSheetByNameOrIndex(XSSFWorkbook workbook, String sheetNameOrIndex, Logger logger) { // shared implementation } public static File downloadFileFromUrl(String url, Logger logger) { // shared implementation } }Also applies to: 254-272
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
excelActions_cloud/src/main/java/com/testsigma/addons/web/StoreNonEmptyCellCountForColumnOrRow.java(1 hunks)excelActions_cloud/src/main/java/com/testsigma/addons/web/StoreNonEmptyCellCountWithSheet.java(1 hunks)
🔇 Additional comments (3)
excelActions_cloud/src/main/java/com/testsigma/addons/web/StoreNonEmptyCellCountWithSheet.java (1)
1-28: LGTM on class structure and annotations.The action text, description, and field annotations are well-defined with proper references and allowed values.
excelActions_cloud/src/main/java/com/testsigma/addons/web/StoreNonEmptyCellCountForColumnOrRow.java (2)
123-154: Column resolution logic is well-designed.The three-tier approach (numeric → Excel letters → header name) with clear logging is user-friendly and robust.
227-252: LGTM on cell counting methods.The
countNonEmptyCellsInColumnandcountNonEmptyCellsInRowmethods correctly iterate and check for non-empty cells.
please review this addon and publish as PUBLIC
Addon name : ExcelActions_cloud
Addon accont: https://jarvis.testsigma.com/ui/tenants/2817/addons
Jira: https://testsigma.atlassian.net/browse/CUS-9450
fix
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.