Enhance Form Submission Export Functionality #657
Conversation
WalkthroughThe pull request introduces changes to the form submission export functionality across multiple files. The modifications allow for more dynamic control over exported data by enabling users to specify which columns should be included in the CSV export. The changes involve updating the Changes
Sequence DiagramsequenceDiagram
participant Client as Form Submissions Component
participant API as Export Controller
participant Database as Form Data Storage
Client->>API: POST /{id}/submissions/export
API->>Database: Retrieve Form Submissions
API-->>Client: CSV with Selected Columns
Poem
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: 0
🧹 Nitpick comments (2)
api/app/Http/Controllers/Forms/FormSubmissionController.php (1)
64-75: Dynamic column export logic looks solid.
Filtering and mapping columns to$filteredDatabased on$displayColumnsis a neat approach. However, consider validating the existence of$columnkeys in your$formattedDatato avoid edge cases if the request or data changes.client/components/open/forms/components/FormSubmissions.vue (1)
336-342: POST request with column data fosters flexibility but requires server validation.
The new body parameter enhances dynamic exports. Ensure the backend enforces security checks and handles empty or largecolumnsarrays gracefully.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
api/app/Http/Controllers/Forms/FormSubmissionController.php(2 hunks)api/routes/api.php(1 hunks)client/components/open/forms/components/FormSubmissions.vue(2 hunks)
🔇 Additional comments (7)
api/app/Http/Controllers/Forms/FormSubmissionController.php (5)
17-17: Good addition of the Request import for improved handling of input data.
This import is essential for the enhanced export functionality.
50-50: Method signature update is aligned with the POST approach.
The change frompublic function export(string $id)topublic function export(Request $request, string $id)allows dynamic input parameters.
56-56: Security check: Validate$request->columnsbefore usage.
Ensure the request contains the expected data type (array) and handle any malformed input to prevent errors or potential injection.
81-81: Filename customization is clear and consistent with this approach.
Appending-submission-data.csvis straightforward and improves clarity.
88-89: Check for consistency in file paths.
The dynamic file path is carefully constructed. Ensure consistency with the rest of your file downloads to avoid confusion around leading/trailing slashes.client/components/open/forms/components/FormSubmissions.vue (1)
216-216: Removing the leading slash is correct for consistent routing.
Make sure your other API calls also follow the same pattern if needed.api/routes/api.php (1)
164-164: Switching from GET to POST aligns with sending request payload.
This ensures that large or sensitive data can be provided without URL length constraints and is more secure.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/app/Http/Controllers/Forms/FormSubmissionController.php(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build the Nuxt app
🔇 Additional comments (6)
api/app/Http/Controllers/Forms/FormSubmissionController.php (6)
17-17: Nice import for dependency injection.
No issues here; usingRequestas a parameter is cleaner than older style approaches.
50-50: The parameter validation requirement has been addressed.
It appears you’ve addressed the previous feedback (“Need to validate the new param you added”) by adding validation below.
61-61: Ensure columns exist in the form.
This mirrors the existing comment about verifying that each requested column exists in the form. Consider adding a check to confirm the columns in$request->columnsmatch actual form fields.
69-80: Be cautious with fuzzy matching.
Usingstr_contains($key, $column)could cause collisions if multiple fields contain the same substring. Consider a stricter match to avoid inadvertently matching partial names.
86-86: CSV export logic looks good.
Returning the CSV with a consistent file name is clear and user-friendly.
93-94: File name handling review.
You may want to ensure thefileNamestring cannot expose path traversal or invalid file references. The call toStorage::exists($fileName)helps, but further sanitization might be prudent to prevent malicious path input.
| $request->validate([ | ||
| 'columns' => 'required|array', | ||
| 'columns.*' => 'boolean' | ||
| ]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider validating recognized columns.
Currently, you validate the columns array and enforce boolean values, but you might also verify that each column key is valid for the form. This ensures you don’t export or reveal unexpected data columns.
There was a problem hiding this comment.
@chiragchhatrala that's what I meant here. please create a dedicated request validation class, checking that it's an array of existing column for this form
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| $request->validate([ | ||
| 'columns' => 'required|array', | ||
| 'columns.*' => 'boolean' | ||
| ]); |
There was a problem hiding this comment.
@chiragchhatrala that's what I meant here. please create a dedicated request validation class, checking that it's an array of existing column for this form
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
api/tests/Feature/Forms/FormSubmissionExportTest.php (2)
7-51: Consider verifying CSV contents in addition to headers.
While it's great that you check for successful status and content-disposition headers, you might also want to parse and validate the CSV data to ensure that all selected columns and corresponding values are exported as expected. This would provide a more robust test against potential issues with CSV generation or data formatting.// After asserting headers, parse the CSV content for deeper validation. + $csvContent = $response->streamedContent(); + $lines = explode("\n", trim($csvContent)); + // For example, verify you have the correct column headers and data rows + // This is just a sample check: + $this->assertStringContainsString('name_field,email_field,created_at', $lines[0]); + $this->assertStringContainsString('John Doe,[email protected]', $lines[1]); + $this->assertStringContainsString('Jane Smith,[email protected]', $lines[2]);
79-98: Confirm that returning “Unauthenticated” is intended behavior.
Since the code checks for an unauthorized export from another user’s form, consider returning an HTTP 403 “Forbidden” if the user is authenticated but lacks permission, instead of “Unauthenticated” (401). Ensure consistency with your overall authentication and authorization flow.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/tests/Feature/Forms/FormSubmissionExportTest.php(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build the Nuxt app
🔇 Additional comments (1)
api/tests/Feature/Forms/FormSubmissionExportTest.php (1)
53-77: Tests for invalid columns look good.
The test accurately checks for a 422 validation error and ensures the 'columns' field triggers a validation issue.
Summary by CodeRabbit
New Features
Changes
Improvements