Partial submissions #705
Partial submissions #705
Conversation
WalkthroughThis pull request introduces functionality for handling partial form submissions. Backend components, including controllers and models, are updated to differentiate between complete and partial submissions, with new validation rules and database migrations. Frontend components integrate logic for managing and synchronizing partial submission data. Overall, the changes improve the tracking, validation, and presentation of forms with partial data, ensuring that partial submissions are processed and displayed appropriately. Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🪧 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: 4
🧹 Nitpick comments (5)
api/app/Models/Forms/FormSubmission.php (1)
21-27: Add enum cast for the status field.The
statusfield is defined as an enum in the database but is not cast in the model. Consider adding an enum cast for type safety.protected function casts(): array { return [ 'data' => 'array', 'completion_time' => 'integer', + 'status' => \Illuminate\Database\Eloquent\Enums\Status::class, ]; }client/components/open/forms/components/FormStats.vue (1)
121-126: Verify color contrast for accessibility.The yellow color used for partial submissions (rgba(255, 193, 7, 1)) should be tested for sufficient contrast against the background.
#!/usr/bin/env python3 import colorsys def get_relative_luminance(r, g, b): r = r / 255 if r <= 255 else 1 g = g / 255 if g <= 255 else 1 b = b / 255 if b <= 255 else 1 r = r / 12.92 if r <= 0.03928 else ((r + 0.055) / 1.055) ** 2.4 g = g / 12.92 if g <= 0.03928 else ((g + 0.055) / 1.055) ** 2.4 b = b / 12.92 if b <= 0.03928 else ((b + 0.055) / 1.055) ** 2.4 return 0.2126 * r + 0.7152 * g + 0.0722 * b # Calculate contrast ratio with white background yellow_luminance = get_relative_luminance(255, 193, 7) white_luminance = get_relative_luminance(255, 255, 255) contrast_ratio = (white_luminance + 0.05) / (yellow_luminance + 0.05) print(f"Contrast ratio with white background: {contrast_ratio:.2f}") print(f"Meets WCAG AA for large text: {contrast_ratio >= 3}") print(f"Meets WCAG AA for normal text: {contrast_ratio >= 4.5}")[skip_cloning]
Consider using a more accessible color scheme:
- backgroundColor: "rgba(255, 193, 7, 1)", - borderColor: "rgba(255, 193, 7, 1)", + backgroundColor: "rgba(245, 158, 11, 1)", // Amber-600 + borderColor: "rgba(245, 158, 11, 1)",api/app/Http/Controllers/Forms/PublicFormController.php (1)
99-113: Consider adding transaction for data consistency.While the logic for handling partial submissions is correct, consider wrapping the database operations in a transaction to ensure data consistency.
- if ($isPartial) { - $submissionResponse = $form->submissions()->updateOrCreate([ - 'id' => $submissionId - ], [ - 'data' => $request->all(), - 'status' => FormSubmission::STATUS_PARTIAL - ]); - $submissionId = $submissionResponse->id; + if ($isPartial) { + $submissionId = DB::transaction(function () use ($form, $submissionId, $request) { + $submissionResponse = $form->submissions()->updateOrCreate([ + 'id' => $submissionId + ], [ + 'data' => $request->all(), + 'status' => FormSubmission::STATUS_PARTIAL + ]); + return $submissionResponse->id; + });client/components/open/forms/components/FormSubmissions.vue (1)
135-140: Consider using constants for status values.To maintain consistency and prevent typos, consider defining the status values as constants.
+const SUBMISSION_STATUS = { + ALL: 'all', + COMPLETED: 'completed', + PARTIAL: 'partial' +}; data() { return { statusList: [ - { label: 'All', value: 'all' }, - { label: 'Completed', value: 'completed' }, - { label: 'Partial', value: 'partial' } + { label: 'All', value: SUBMISSION_STATUS.ALL }, + { label: 'Completed', value: SUBMISSION_STATUS.COMPLETED }, + { label: 'Partial', value: SUBMISSION_STATUS.PARTIAL } ], - selectedStatus: 'all', + selectedStatus: SUBMISSION_STATUS.ALL,client/components/open/tables/OpenTable.vue (1)
100-110: Consider adding a tooltip to explain the status meanings.The status badge implementation is good, but users might need help understanding what "partial" vs. "complete" means.
Add a tooltip to explain the status:
<UBadge :label="row.status" :color="row.status === 'partial' ? 'yellow' : 'green'" variant="soft" + :tooltip="row.status === 'partial' ? 'Form is partially filled' : 'Form is complete'" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
api/app/Http/Controllers/Forms/FormStatsController.php(2 hunks)api/app/Http/Controllers/Forms/PublicFormController.php(1 hunks)api/app/Http/Requests/AnswerFormRequest.php(1 hunks)api/app/Http/Requests/UserFormRequest.php(1 hunks)api/app/Http/Resources/FormSubmissionResource.php(1 hunks)api/app/Jobs/Form/StoreFormSubmissionJob.php(1 hunks)api/app/Models/Forms/Form.php(3 hunks)api/app/Models/Forms/FormSubmission.php(1 hunks)api/database/migrations/2025_02_14_073642_add_partial_submissions_to_form_submissions.php(1 hunks)client/components/open/forms/OpenCompleteForm.vue(3 hunks)client/components/open/forms/OpenForm.vue(4 hunks)client/components/open/forms/components/FormStats.vue(2 hunks)client/components/open/forms/components/FormSubmissions.vue(4 hunks)client/components/open/forms/components/form-components/FormSubmissionSettings.vue(1 hunks)client/components/open/tables/OpenTable.vue(3 hunks)client/composables/forms/pendingSubmission.js(1 hunks)client/composables/forms/usePartialSubmission.js(1 hunks)client/stores/working_form.js(2 hunks)
🔇 Additional comments (28)
api/app/Models/Forms/FormSubmission.php (2)
12-13: LGTM! Constants provide type safety.The constants match the database schema and provide type safety when working with submission statuses.
15-19: LGTM! Mass assignment is properly configured.The
statusfield is correctly added to the$fillablearray, allowing mass assignment of the new status field.api/database/migrations/2025_02_14_073642_add_partial_submissions_to_form_submissions.php (2)
11-25: LGTM! Migration is well-structured.The migration:
- Properly defines column types and defaults
- Positions columns logically using
after- Indexes the
statuscolumn for better query performance
30-39: LGTM! Rollback is properly implemented.The
downmethod correctly drops both added columns, providing a clean rollback path.client/composables/forms/pendingSubmission.js (1)
48-57: LGTM! Exposing the key enhances composable reusability.The
formPendingSubmissionKeyis now properly exposed, allowing other components to access and utilize the computed key.api/app/Http/Resources/FormSubmissionResource.php (1)
40-47: LGTM! Status is properly included in the resource.The
statusfield is correctly added to the extra data array, maintaining consistency with the existing pattern of including additional data only for non-public access.api/app/Http/Controllers/Forms/PublicFormController.php (2)
93-97: LGTM! Secure handling of submission IDs.The code correctly uses Hashids to decode the submission hash, providing a secure way to reference submissions.
118-124: LGTM! Clean data handling.The code correctly manages submission data by removing extra fields and adding the submission ID when needed.
api/app/Http/Requests/UserFormRequest.php (1)
84-84: LGTM! Proper validation rule.The boolean validation for
enable_partial_submissionsis correctly placed in the submission settings section.client/components/open/forms/components/FormSubmissions.vue (2)
45-51: LGTM! Conditional rendering of status filter.The status filter is correctly shown only when partial submissions are enabled.
163-167: LGTM! Efficient filtering and reactivity.The filtering logic is correctly implemented and properly triggers data changes when the status changes.
Also applies to: 191-193
client/components/open/forms/components/form-components/FormSubmissionSettings.vue (1)
29-35: LGTM! Clear and consistent UI.The toggle switch follows the existing pattern and includes helpful explanatory text for users.
api/app/Models/Forms/Form.php (3)
96-96: LGTM! Addedenable_partial_submissionsto fillable array.The addition allows mass assignment of this attribute, which is necessary for the partial submissions feature.
114-115: LGTM! Added type casting for attributes.The changes properly cast:
seo_metaas objectenable_partial_submissionsas boolean, ensuring type safety
179-179: LGTM! Updated submissions count to only include completed submissions.The change ensures that only completed submissions are counted, which is the correct behavior when tracking form completion statistics.
api/app/Jobs/Form/StoreFormSubmissionJob.php (3)
71-74: LGTM! Added submission ID handling.The code properly retrieves and sets the submission ID from the submission data.
80-80: LGTM! Set status for updated submissions.The code correctly sets the status to completed when updating an existing submission.
87-87: LGTM! Set status for new submissions.The code correctly sets the status to completed when creating a new submission.
api/app/Http/Requests/AnswerFormRequest.php (1)
57-61:❓ Verification inconclusive
Verify security implications of skipping validation for partial submissions.
While skipping validation for partial submissions is necessary for the feature, it could potentially lead to security issues if not handled properly in downstream code.
Run the following script to check for any validation or security measures in the form submission flow:
🏁 Script executed:
#!/bin/bash # Description: Check for validation and security measures in form submission flow # Check for validation in controllers rg -A 5 "is_partial|STATUS_PARTIAL" app/Http/Controllers/ # Check for security middleware rg -A 5 "middleware.*auth" app/Http/Controllers/Forms/Length of output: 246
Security Check for Partial Submissions
The current implementation in
api/app/Http/Requests/AnswerFormRequest.phpat lines 57–61 correctly skips validation for requests flagged as partial. However, our automated search using the provided script did not return any evidence of additional validation or security measures in downstream controllers (the paths queried were not found in the repository). This leaves some uncertainty about whether other parts of the submission flow adequately handle the security implications of bypassing validation.Action Items:
- Please verify manually (or with targeted searches in the complete codebase) that all downstream consumers of this request—be it controllers, middleware, or service layers—properly enforce necessary security and sanitization measures for partial submissions.
- Ensure that any security checks or context-specific validations that are required for partial submissions are implemented externally since full request validation is skipped in this form request.
client/components/open/forms/OpenCompleteForm.vue (3)
207-208: LGTM! Added imports for partial submission handling.The code properly imports the necessary composables for handling partial submissions.
239-239: LGTM! Added partial submission setup.The code correctly initializes the partial submission handler in the setup function.
313-316: LGTM! Added cleanup for partial submissions.The code properly stops synchronization of partial submissions when the form is submitted.
client/components/open/tables/OpenTable.vue (2)
31-41: LGTM! The status column header is well-implemented.The implementation correctly uses conditional rendering and maintains consistent styling with other columns.
251-253: LGTM! The computed property is correctly implemented.The
hasStatuscomputed property correctly checks for theenable_partial_submissionsflag.client/components/open/forms/OpenForm.vue (4)
132-133: LGTM! The imports are correctly organized.The imports are well-structured and maintain consistent spacing.
190-190: LGTM! The partial submission setup is correctly initialized.The
usePartialSubmissioncomposable is properly initialized with the required dependencies.
384-386:❓ Verification inconclusive
Verify submission hash generation.
The submission hash is set correctly, but we should verify the hash generation logic to ensure uniqueness.
Run this script to check the hash generation implementation:
🏁 Script executed:
#!/bin/bash # Description: Check submission hash generation logic # Search for hash generation implementation ast-grep --pattern $'getSubmissionHash($_) { $$$ }'Length of output: 54
Action: Verify the submission hash generation logic directly
The previous script did not return output, so we couldn’t locate the implementation details via the
ast-grepcommand. Please manually verify that thegetSubmissionHash()function—likely defined within your working form store—is correctly implemented to generate unique submission hashes.
- Confirm that the implementation of
getSubmissionHash()guarantees uniqueness.- If the function is defined in a non-standard format or in an unexpected location, consider trying an alternative search (e.g., using
rg "getSubmissionHash(" -A 5) to locate and review its logic.
361-363:❓ Verification inconclusive
Verify cleanup of partial submission sync.
The lifecycle hooks correctly handle the sync start/stop, but we should verify that the sync is properly cleaned up in all scenarios.
Run this script to check for potential memory leaks:
Also applies to: 365-368
🏁 Script executed:
#!/bin/bash # Description: Check for proper cleanup in all component unmount scenarios # Search for other components using partialSubmission rg -A 5 "usePartialSubmission" # Search for cleanup patterns in composable rg "stopSync|cleanup|dispose" "client/composables/forms"Length of output: 2959
Action: Confirm that the partial submission sync cleanup runs on every unmount.
Our investigation shows that the composable (client/composables/forms/usePartialSubmission.js) clearly defines and exports a cleanup function (stopSync), and it’s used in bothOpenForm.vueandOpenCompleteForm.vue. However, it isn’t immediately apparent thatstopSyncis automatically invoked when the component unmounts (or when specific conditions, such as admin preview mode, occur).Please ensure that:
- An appropriate lifecycle hook (e.g.,
onBeforeUnmount) is in place—either within the composable or in the component—to callstopSync()and properly stop the sync mechanism.- Cleanup is reliably executed in all scenarios to prevent potential memory leaks.
| $periodStats['submissions'][$date] = $form->submissions()->whereDate('created_at', $dateObj)->where('status', FormSubmission::STATUS_COMPLETED)->count(); | ||
| $periodStats['partial_submissions'][$date] = $form->submissions()->whereDate('created_at', $dateObj)->where('status', FormSubmission::STATUS_PARTIAL)->count(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
❓ Verification inconclusive
Verify FormSubmission status constants and add error handling.
The code assumes the existence of FormSubmission::STATUS_COMPLETED and FormSubmission::STATUS_PARTIAL constants. Additionally, database queries should include error handling.
Add try-catch blocks for database queries:
- $periodStats['submissions'][$date] = $form->submissions()->whereDate('created_at', $dateObj)->where('status', FormSubmission::STATUS_COMPLETED)->count();
- $periodStats['partial_submissions'][$date] = $form->submissions()->whereDate('created_at', $dateObj)->where('status', FormSubmission::STATUS_PARTIAL)->count();
+ try {
+ $periodStats['submissions'][$date] = $form->submissions()
+ ->whereDate('created_at', $dateObj)
+ ->where('status', FormSubmission::STATUS_COMPLETED)
+ ->count();
+ $periodStats['partial_submissions'][$date] = $form->submissions()
+ ->whereDate('created_at', $dateObj)
+ ->where('status', FormSubmission::STATUS_PARTIAL)
+ ->count();
+ } catch (\Exception $e) {
+ \Log::error('Error fetching form stats: ' . $e->getMessage());
+ $periodStats['submissions'][$date] = 0;
+ $periodStats['partial_submissions'][$date] = 0;
+ }🏁 Script executed:
#!/bin/bash
# Verify FormSubmission status constants
ast-grep --pattern 'class FormSubmission {
$$$
const STATUS_COMPLETED = $_;
const STATUS_PARTIAL = $_;
$$$
}'Length of output: 119
Verify FormSubmission Constants and Add Error Handling
- Please manually verify that the
FormSubmissionclass defines bothSTATUS_COMPLETEDandSTATUS_PARTIALconstants. Our initial AST search did not return output—use a text search (e.g., withrg) to confirm their existence. - Wrap the database queries in a try-catch block to handle exceptions and prevent runtime failures during query execution. For example:
- $periodStats['submissions'][$date] = $form->submissions()->whereDate('created_at', $dateObj)->where('status', FormSubmission::STATUS_COMPLETED)->count();
- $periodStats['partial_submissions'][$date] = $form->submissions()->whereDate('created_at', $dateObj)->where('status', FormSubmission::STATUS_PARTIAL)->count();
+ try {
+ $periodStats['submissions'][$date] = $form->submissions()
+ ->whereDate('created_at', $dateObj)
+ ->where('status', FormSubmission::STATUS_COMPLETED)
+ ->count();
+ $periodStats['partial_submissions'][$date] = $form->submissions()
+ ->whereDate('created_at', $dateObj)
+ ->where('status', FormSubmission::STATUS_PARTIAL)
+ ->count();
+ } catch (\Exception $e) {
+ \Log::error('Error fetching form stats: ' . $e->getMessage());
+ $periodStats['submissions'][$date] = 0;
+ $periodStats['partial_submissions'][$date] = 0;
+ }To further assist verification, you may run:
Please ensure these changes align with your codebase before merging.
🏁 Script executed:
[run_scripts]
#!/bin/bash
# Search for the FormSubmission class and its status constants using rg:
echo "Searching for FormSubmission class definition:"
rg --ignore-case "class\s+FormSubmission" || echo "No FormSubmission class found."
echo "Searching for STATUS_COMPLETED constant:"
rg --ignore-case "const\s+STATUS_COMPLETED" || echo "STATUS_COMPLETED constant not found."
echo "Searching for STATUS_PARTIAL constant:"
rg --ignore-case "const\s+STATUS_PARTIAL" || echo "STATUS_PARTIAL constant not found."Length of output: 1713
Constants Verified – Please Add Error Handling to Database Queries
The status constants FormSubmission::STATUS_COMPLETED and FormSubmission::STATUS_PARTIAL have been confirmed in api/app/Models/Forms/FormSubmission.php. Since these constants exist, the primary focus should be on enhancing the robustness of the database queries by wrapping them in a try-catch block, as outlined below:
- Verified:
FormSubmission::STATUS_COMPLETEDexists.FormSubmission::STATUS_PARTIALexists.
- Action Required:
- Add error handling (a try-catch block) around the database queries to ensure exceptions are caught and logged.
Please update your code as follows:
- $periodStats['submissions'][$date] = $form->submissions()->whereDate('created_at', $dateObj)->where('status', FormSubmission::STATUS_COMPLETED)->count();
- $periodStats['partial_submissions'][$date] = $form->submissions()->whereDate('created_at', $dateObj)->where('status', FormSubmission::STATUS_PARTIAL)->count();
+ try {
+ $periodStats['submissions'][$date] = $form->submissions()
+ ->whereDate('created_at', $dateObj)
+ ->where('status', FormSubmission::STATUS_COMPLETED)
+ ->count();
+ $periodStats['partial_submissions'][$date] = $form->submissions()
+ ->whereDate('created_at', $dateObj)
+ ->where('status', FormSubmission::STATUS_PARTIAL)
+ ->count();
+ } catch (\Exception $e) {
+ \Log::error('Error fetching form stats: ' . $e->getMessage());
+ $periodStats['submissions'][$date] = 0;
+ $periodStats['partial_submissions'][$date] = 0;
+ }Please verify that the try-catch implementation aligns with your overall error handling strategy before merging.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $periodStats['submissions'][$date] = $form->submissions()->whereDate('created_at', $dateObj)->where('status', FormSubmission::STATUS_COMPLETED)->count(); | |
| $periodStats['partial_submissions'][$date] = $form->submissions()->whereDate('created_at', $dateObj)->where('status', FormSubmission::STATUS_PARTIAL)->count(); | |
| try { | |
| $periodStats['submissions'][$date] = $form->submissions() | |
| ->whereDate('created_at', $dateObj) | |
| ->where('status', FormSubmission::STATUS_COMPLETED) | |
| ->count(); | |
| $periodStats['partial_submissions'][$date] = $form->submissions() | |
| ->whereDate('created_at', $dateObj) | |
| ->where('status', FormSubmission::STATUS_PARTIAL) | |
| ->count(); | |
| } catch (\Exception $e) { | |
| \Log::error('Error fetching form stats: ' . $e->getMessage()); | |
| $periodStats['submissions'][$date] = 0; | |
| $periodStats['partial_submissions'][$date] = 0; | |
| } |
| const stopSync = () => { | ||
| if (syncInterval) { | ||
| clearInterval(syncInterval) | ||
| syncInterval = null | ||
| } | ||
| if (syncTimeout) { | ||
| clearTimeout(syncTimeout) | ||
| syncTimeout = null | ||
| } | ||
|
|
||
| // Remove event listeners | ||
| document.removeEventListener('visibilitychange', handleVisibilityChange) | ||
| window.removeEventListener('blur', handleBlur) | ||
| window.removeEventListener('beforeunload', handleBeforeUnload) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure cleanup of resources in error scenarios.
The stopSync function should be called in error scenarios to prevent memory leaks and ensure proper cleanup.
const stopSync = () => {
if (syncInterval) {
clearInterval(syncInterval)
syncInterval = null
}
if (syncTimeout) {
clearTimeout(syncTimeout)
syncTimeout = null
}
// Remove event listeners
document.removeEventListener('visibilitychange', handleVisibilityChange)
window.removeEventListener('blur', handleBlur)
window.removeEventListener('beforeunload', handleBeforeUnload)
+
+ // Reset retry count
+ retryCount = 0;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const stopSync = () => { | |
| if (syncInterval) { | |
| clearInterval(syncInterval) | |
| syncInterval = null | |
| } | |
| if (syncTimeout) { | |
| clearTimeout(syncTimeout) | |
| syncTimeout = null | |
| } | |
| // Remove event listeners | |
| document.removeEventListener('visibilitychange', handleVisibilityChange) | |
| window.removeEventListener('blur', handleBlur) | |
| window.removeEventListener('beforeunload', handleBeforeUnload) | |
| } | |
| const stopSync = () => { | |
| if (syncInterval) { | |
| clearInterval(syncInterval) | |
| syncInterval = null | |
| } | |
| if (syncTimeout) { | |
| clearTimeout(syncTimeout) | |
| syncTimeout = null | |
| } | |
| // Remove event listeners | |
| document.removeEventListener('visibilitychange', handleVisibilityChange) | |
| window.removeEventListener('blur', handleBlur) | |
| window.removeEventListener('beforeunload', handleBeforeUnload) | |
| // Reset retry count | |
| retryCount = 0; | |
| } |
| } catch (error) { | ||
| console.error('Failed to sync partial submission', error) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling with retry mechanism.
The error handling only logs the error without any retry mechanism. For better reliability, implement exponential backoff retries.
+ let retryCount = 0;
+ const MAX_RETRIES = 3;
+ const RETRY_DELAY = 1000;
+
+ const retrySync = async (error) => {
+ if (retryCount < MAX_RETRIES) {
+ retryCount++;
+ const delay = RETRY_DELAY * Math.pow(2, retryCount - 1);
+ console.warn(`Retrying sync attempt ${retryCount} after ${delay}ms`);
+ await new Promise(resolve => setTimeout(resolve, delay));
+ return syncToServer();
+ }
+ throw error;
+ }
+
try {
const submissionHash = workingFormStore.getSubmissionHash(pendingSubmission.formPendingSubmissionKey.value)
const response = await opnFetch(`/forms/${form.slug}/answer`, {
method: "POST",
body: {
...formData.value.data(),
'is_partial': true,
'submission_hash': submissionHash
}
})
if (response.submission_hash) {
workingFormStore.setSubmissionHash(pendingSubmission.formPendingSubmissionKey.value, response.submission_hash)
}
+ retryCount = 0;
} catch (error) {
- console.error('Failed to sync partial submission', error)
+ console.error('Failed to sync partial submission', error);
+ return retrySync(error);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (error) { | |
| console.error('Failed to sync partial submission', error) | |
| } | |
| } | |
| async function syncToServer() { | |
| let retryCount = 0; | |
| const MAX_RETRIES = 3; | |
| const RETRY_DELAY = 1000; | |
| const retrySync = async (error) => { | |
| if (retryCount < MAX_RETRIES) { | |
| retryCount++; | |
| const delay = RETRY_DELAY * Math.pow(2, retryCount - 1); | |
| console.warn(`Retrying sync attempt ${retryCount} after ${delay}ms`); | |
| await new Promise(resolve => setTimeout(resolve, delay)); | |
| return syncToServer(); | |
| } | |
| throw error; | |
| } | |
| try { | |
| const submissionHash = workingFormStore.getSubmissionHash( | |
| pendingSubmission.formPendingSubmissionKey.value | |
| ); | |
| const response = await opnFetch(`/forms/${form.slug}/answer`, { | |
| method: "POST", | |
| body: { | |
| ...formData.value.data(), | |
| is_partial: true, | |
| submission_hash: submissionHash | |
| } | |
| }); | |
| if (response.submission_hash) { | |
| workingFormStore.setSubmissionHash( | |
| pendingSubmission.formPendingSubmissionKey.value, | |
| response.submission_hash | |
| ); | |
| } | |
| retryCount = 0; | |
| } catch (error) { | |
| console.error('Failed to sync partial submission', error); | |
| return retrySync(error); | |
| } | |
| } |
There was a problem hiding this comment.
Hey please check the comments. Also:
- let's make this a Pro feature
- every time I reload the form, a new partial submission is created, but since local storage save is enabled it shouldn't. It should only be like this when local storage is not enabled
- files/signature etc don't work. it should upload them and move them the first time it's saved after submisison is created, then after save, dataform should be update to include the file link instead of key. Just like we do when editing existing submissions
thanks!
api/database/migrations/2025_02_14_073642_add_partial_submissions_to_form_submissions.php
Outdated
Show resolved
Hide resolved
api/database/migrations/2025_02_14_073642_add_partial_submissions_to_form_submissions.php
Outdated
Show resolved
Hide resolved
api/database/migrations/2025_02_14_073642_add_partial_submissions_to_form_submissions.php
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| const handleBeforeUnload = () => { | ||
| syncToServer() |
There was a problem hiding this comment.
This doesn't seem to work for me, maybe we need to await? Do we need to display something like an alert message in the meantime? it doesn't send the data before saving
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
api/app/Http/Controllers/Forms/PublicFormController.php (1)
99-113: 🛠️ Refactor suggestionRefactor the answer method to improve readability and maintainability.
The method is becoming too complex with multiple responsibilities. Consider extracting the partial submission handling logic into a separate private method.
Also, using
$request->all()might include unwanted or sensitive data in the submission. Consider using validated data instead:$submissionResponse = $form->submissions()->updateOrCreate([ 'id' => $submissionId ], [ - 'data' => $request->all(), + 'data' => $request->validated(), 'status' => FormSubmission::STATUS_PARTIAL ]);
🧹 Nitpick comments (1)
api/app/Http/Controllers/Forms/PublicFormController.php (1)
94-97: Improve hash decoding safety.The current code handles null values well, but there's a potential issue when Hashids::decode returns an empty array. Consider adding an explicit check for array emptiness.
if ($submissionHash) { $submissionHash = Hashids::decode($submissionHash); - $submissionId = (int)($submissionHash[0] ?? null); + $submissionId = !empty($submissionHash) ? (int)$submissionHash[0] : null; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
api/app/Http/Controllers/Forms/PublicFormController.php(1 hunks)client/components/open/forms/components/form-components/FormSubmissionSettings.vue(1 hunks)client/components/open/tables/OpenTable.vue(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- client/components/open/forms/components/form-components/FormSubmissionSettings.vue
🔇 Additional comments (6)
client/components/open/tables/OpenTable.vue (3)
31-41: LGTM! Good implementation of the status header.The header styling and conditional rendering look correct, making the column display only when partial submissions are enabled for professional forms.
100-110: Please capitalize the status label and set the badge size to xs.The status badge implementation is almost there, but you need to address the previous review comments.
Apply this diff to fix the issues:
<UBadge - :label="row.status" + :label="row.status.charAt(0).toUpperCase() + row.status.slice(1)" :color="row.status === 'partial' ? 'yellow' : 'green'" variant="soft" + size="xs" />
251-253: LGTM! Well-implemented conditional check.The computed property properly checks both conditions needed for showing the status column: the form must be in a professional state and have partial submissions enabled.
api/app/Http/Controllers/Forms/PublicFormController.php (3)
117-120: LGTM! Properly cleaning up request data.The code correctly removes fields that are handled separately from the submission data.
121-124: Consider using a setter method in the job.While this approach works, it would be more consistent to use a setter method like
setSubmissionIdin the StoreFormSubmissionJob class, as mentioned in previous comments.
133-135: Refactor job dispatching for consistency.As suggested in previous comments, consider refactoring to use
$job->dispatch()instead of directly callingStoreFormSubmissionJob::dispatch()for better consistency.} else { - StoreFormSubmissionJob::dispatch($form, $submissionData, $completionTime); + $job->dispatch(); }
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
client/composables/forms/usePartialSubmission.js (1)
60-63: Consider asynchronous constraints inhandleBeforeUnload.
Browsers typically do not await async calls duringbeforeunload, so the request might not complete if the user immediately closes the tab. You could evaluate sending a synchronous message or using the Beacon API for better reliability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/components/open/forms/OpenForm.vue(6 hunks)client/composables/forms/usePartialSubmission.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- client/components/open/forms/OpenForm.vue
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build the Nuxt app
🔇 Additional comments (13)
client/composables/forms/usePartialSubmission.js (13)
1-3: No issues with imports.
Everything looks good with the imports and setup.
4-6: Utilizing a reactive Map for submission hashes is fine.
This approach is clear and straightforward for mapping each form instance to its submission hash.
7-9: Well-structured function signature.
DefiningusePartialSubmission(form, formData = {})with a default parameter forformDatamaintains flexibility.
10-13: Interval-based synchronization is acceptable for partial submissions.
No immediate concerns with setting the default sync interval at 30 seconds.
14-17: Getter is straightforward.
getSubmissionHashsimply retrieves the stored hash keyed by the form’s pending submission key.
18-20: Setter is straightforward.
setSubmissionHashupdates the stored hash in a clear, succinct manner.
22-27: Debounced approach is appropriate.
The 1-second debounce prevents excessive server calls when the user is actively modifying form data.
29-47: Revisit error handling and potential concurrency.Currently, the catch block (lines 44-46) only logs the error. In a high-latency environment, overlapping sync calls or transient errors could cause data loss or confusion.
Additionally, consider implementing a retry or queue mechanism to handle transient failures and avoid concurrency pitfalls (e.g., two sync calls in flight).
49-55: Visibility change handler is correct.
Debounced sync on tab backgrounding helps ensure partial data gets saved if the browser tab goes hidden.
56-59: Blur event handling is fine.
Again, performing a debounced sync on window blur can prevent data loss if the user quickly navigates away.
64-80: Startup logic is solid.
Starting with an immediate sync and then setting an interval for recurring syncs works well.
81-95: CallstopSyncon critical errors if needed.As previously suggested, consider stopping the sync in unrecoverable error scenarios to avoid resource leaks or repeated failures.
97-104: Return object is well-organized.
Exposing clear methods fosters reusability in other components.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
api/app/Http/Controllers/Forms/PublicFormController.php (1)
88-148: 🛠️ Refactor suggestionImplement proper error handling for database operations.
The current implementation lacks proper error handling for database operations. Both
handlePartialSubmissionsand the mainanswermethod interact with the database but don't catch potential exceptions. Consider wrapping these operations in try/catch blocks to handle failures gracefully.Example implementation:
private function handlePartialSubmissions(Request $request, $submissionId) { $form = $request->form; + try { $submissionResponse = $form->submissions()->updateOrCreate([ 'id' => $submissionId ], [ 'data' => $request->all(), 'status' => FormSubmission::STATUS_PARTIAL ]); $submissionId = $submissionResponse->id; return $this->success([ 'message' => 'Partial submission saved', 'submission_hash' => Hashids::encode($submissionId) ]); + } catch (\Exception $e) { + \Log::error('Failed to save partial submission: ' . $e->getMessage()); + return $this->error([ + 'message' => 'Failed to save partial submission', + ], 500); + } }
🧹 Nitpick comments (1)
api/app/Http/Controllers/Forms/PublicFormController.php (1)
116-119: Add error handling for unavailable feature.If a user tries to use partial submissions on a form that doesn't have the feature enabled (either not pro or the feature toggle is off), the current code silently ignores the partial submission request. Consider returning an appropriate error message.
$isPartial = $request->get('is_partial') ?? false; if ($isPartial) { + if (!$form->is_pro) { + return $this->error([ + 'message' => 'Partial submissions are only available on Pro plans.', + ], 403); + } + if (!$form->enable_partial_submissions) { + return $this->error([ + 'message' => 'Partial submissions are not enabled for this form.', + ], 403); + } return $this->handlePartialSubmissions($request, $submissionId); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/app/Http/Controllers/Forms/PublicFormController.php(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build the Nuxt app
🔇 Additional comments (4)
api/app/Http/Controllers/Forms/PublicFormController.php (4)
88-103: Good implementation of the partial submission handler.The method is well-structured and clearly handles the logic for partial submissions. It properly creates or updates a submission record and returns an appropriate response.
123-130: Good removal of extra fields from submission data.Properly cleaning up the submission data by removing meta fields like
completion_timeandsubmission_hashensures these don't accidentally get stored as form data.
105-148: Refactor the answer method for better readability.This method is getting quite complex with the new additions. As mentioned in a previous review comment, this method is becoming difficult to read and maintain. Consider breaking it down into smaller, focused private methods.
For example:
processSubmissionHash()to handle hash decoding and validationhandleFormSubmission()to handle the core submission logicprepareResponseData()to prepare the responseThis would make the main method more concise and easier to follow.
132-141: Refactor job dispatch pattern.As mentioned in a previous review comment, consider using the instance method for dispatching the job rather than the static method. This would be more consistent with the synchronous case where you're already using the job instance.
if ($formSubmissionProcessor->shouldProcessSynchronously($form)) { $job->handle(); $submissionId = Hashids::encode($job->getSubmissionId()); // Update submission data with generated values for redirect URL $submissionData = $job->getProcessedData(); } else { - StoreFormSubmissionJob::dispatch($form, $submissionData, $completionTime); + dispatch($job); }
- Improve submission ID extraction and decoding across controllers - Add robust handling for submission hash and ID conversion - Enhance metadata processing in StoreFormSubmissionJob - Simplify submission storage logic with clearer metadata extraction - Minor UI improvements in FormSubmissions and OpenTable components
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
api/app/Http/Controllers/Forms/FormSubmissionController.php (1)
69-69: Avoid overshadowing function parameters.The lambda parameter
fn($key)reuses the$keyvariable name, which can cause confusion. Rename the lambda variable for clarity:- $key = collect($formattedData)->keys()->first(fn($key) => str_contains($key, $column)); + $key = collect($formattedData)->keys()->first(fn($colKey) => str_contains($colKey, $column));client/components/open/forms/components/FormSubmissions.vue (2)
45-52: Reorder attributes for style consistency.According to the static analysis warnings, placing
v-ifandv-modelbeforeclassaligns with Vue style guidelines:- <USelectMenu - class="w-32" - v-if="form.enable_partial_submissions" - v-model="selectedStatus" - :options="statusList" - value-attribute="value" - option-attribute="label" /> + <USelectMenu + v-if="form.enable_partial_submissions" + v-model="selectedStatus" + class="w-32" + :options="statusList" + value-attribute="value" + option-attribute="label" />🧰 Tools
🪛 GitHub Check: Run client linters
[warning] 47-47:
Attribute "v-if" should go before "class"
[warning] 48-48:
Attribute "v-model" should go before "class"
136-141: Validate naming consistency of submission statuses.Using "Submitted" →
"completed"and "In Progress" →"partial"is fine if these status labels match backend constants. Otherwise, consider defining an enum or shared constants for concise future maintenance.api/app/Http/Controllers/Forms/PublicFormController.php (1)
128-148: Consolidate repeated job handling logic.Since
$job->handle()and$encodedSubmissionIdassignments are repeated in both branches of the conditional, consider extracting them to reduce duplication:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
api/app/Http/Controllers/Forms/FormSubmissionController.php(3 hunks)api/app/Http/Controllers/Forms/PublicFormController.php(1 hunks)api/app/Jobs/Form/StoreFormSubmissionJob.php(4 hunks)client/components/open/forms/components/FormSubmissions.vue(4 hunks)client/components/open/tables/OpenTable.vue(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- client/components/open/tables/OpenTable.vue
- api/app/Jobs/Form/StoreFormSubmissionJob.php
🧰 Additional context used
🪛 GitHub Check: Run client linters
client/components/open/forms/components/FormSubmissions.vue
[warning] 47-47:
Attribute "v-if" should go before "class"
[warning] 48-48:
Attribute "v-model" should go before "class"
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build the Nuxt app
🔇 Additional comments (8)
api/app/Http/Controllers/Forms/FormSubmissionController.php (2)
39-42: Confirm synchronous processing for StoreFormSubmissionJob.Calling
$job->handle()directly executes the job in a synchronous manner. If the intended design is to leverage Laravel’s queue system for asynchronous processing, consider usingdispatch($job)instead. Otherwise, please verify that running the job synchronously is indeed desired.
58-58: Looks good.The filtering logic is straightforward, ensuring only true-valued columns get displayed.
client/components/open/forms/components/FormSubmissions.vue (2)
164-168: LGTM!Filtering submissions by
selectedStatusis a straightforward and effective approach.
191-193: Watcher usage is correct.Invoking
dataChanged()whenselectedStatusorsearchForm.searchchanges keeps the UI in sync without unnecessary complexity.api/app/Http/Controllers/Forms/PublicFormController.php (4)
88-115: Verify authorization and avoid potential overwrites.
handlePartialSubmissionsperforms anupdateOrCreatethat may overwrite existing completed submissions and does not explicitly check if the current user can modify that submission. Ensure you have proper authorization checks and confirm you want to allow potential overwrite of existing records with partial status.
122-126: Conditional partial submission logic looks good.Restricting partial submissions to users on a pro plan is clear and straightforward.
156-184: Ensure cross-form submission safety.Decoding the submission hash is helpful, but consider verifying that the decoded submission truly belongs to the current form before proceeding. This avoids potential unauthorized cross-form submission manipulations.
188-190: Looks correct.Decoding the submission ID in
fetchSubmissionmatches the approach inprocessSubmissionIdentifiersfor consistent ID handling.
…options - Restructure partial submissions toggle with more descriptive label - Add advanced submission options section with Pro tag - Improve help text for partial submissions feature - Update ProTag with more detailed upgrade modal description
There was a problem hiding this comment.
- When auto save enabled issue: when refreshing the page and finally submitting, a new record is creating instead of finalizing the pending one (creating duplicates)
- if I click fill again, then a pending submission is instantly created even though form is still empty
- still. may issues with signatures and files please check again
- Improve partial submission synchronization in usePartialSubmission composable - Replace interval-based sync with Vue's reactive watch - Add robust handling for different form data input patterns - Implement onBeforeUnmount hook for final sync attempt - Enhance data synchronization reliability and performance
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
api/app/Http/Controllers/Forms/PublicFormController.php (2)
131-146: 🛠️ Refactor suggestionRefactoring needed to improve readability and maintainability.
This method is getting quite long and complex, which was noted in a previous review. Consider breaking down the
answermethod into smaller, focused methods for better maintainability.Additionally, there's inconsistency in how the job is processed. Both conditional branches call
$job->handle()directly, which makes the conditional check for synchronous processing less meaningful.if ($formSubmissionProcessor->shouldProcessSynchronously($form)) { $job->handle(); $encodedSubmissionId = Hashids::encode($job->getSubmissionId()); // Update submission data with generated values for redirect URL $submissionData = $job->getProcessedData(); } else { - $job->handle(); - $encodedSubmissionId = Hashids::encode($job->getSubmissionId()); + // Process asynchronously if needed + $job->dispatch(); + $encodedSubmissionId = Hashids::encode($job->getSubmissionId()); }
156-184:⚠️ Potential issueValidate submission ownership for security.
While the method properly decodes the submission hash/ID, it doesn't verify that the decoded submission ID belongs to the current form. This could potentially allow users to access or modify submissions from other forms.
Add verification to ensure the submission belongs to the current form:
private function processSubmissionIdentifiers(Request $request, array $submissionData): array { // Handle submission hash if present (convert to numeric submission_id) $submissionHash = $request->get('submission_hash'); if ($submissionHash) { $decodedHash = Hashids::decode($submissionHash); if (!empty($decodedHash)) { $submissionId = (int)($decodedHash[0] ?? null); + // Verify the submission belongs to this form + $form = $request->form; + $existingSubmission = $form->submissions()->find($submissionId); + if (!$existingSubmission) { + // Either the submission doesn't exist or doesn't belong to this form + $submissionId = null; + } $submissionData['submission_id'] = $submissionId; } unset($submissionData['submission_hash']); } // Handle string submission_id if present (convert to numeric) if (isset($submissionData['submission_id']) && is_string($submissionData['submission_id']) && !is_numeric($submissionData['submission_id'])) { $decodedId = Hashids::decode($submissionData['submission_id']); if (!empty($decodedId)) { $submissionId = (int)($decodedId[0] ?? null); + // Verify the submission belongs to this form + $form = $request->form; + $existingSubmission = $form->submissions()->find($submissionId); + if (!$existingSubmission) { + // Either the submission doesn't exist or doesn't belong to this form + $submissionId = null; + } $submissionData['submission_id'] = $submissionId; } } return $submissionData; }
🧹 Nitpick comments (2)
api/app/Http/Controllers/Forms/PublicFormController.php (2)
88-115: Good implementation of the partial submissions feature.The
handlePartialSubmissionsmethod is well-structured and follows the single responsibility principle. It properly processes the submission data, marks it as partial, and uses the existing job system for consistency.However, consider using dependency injection for the
FormSubmissionProcessorto maintain consistency with theanswermethod:-private function handlePartialSubmissions(Request $request) +private function handlePartialSubmissions(Request $request, FormSubmissionProcessor $formSubmissionProcessor)This would allow you to leverage the same processor logic for both partial and complete submissions if needed.
188-191: Improve error handling for invalid submission IDs.The current implementation sets
$submissionIdtofalsewhen decoding fails, but the subsequent code at line 193 checks for!$submissionIdwhich will be true for bothfalseand0. While0is unlikely to be a valid ID in most systems, it's better to be explicit.$decodedId = Hashids::decode($submissionId); -$submissionId = !empty($decodedId) ? (int)($decodedId[0]) : false; +$submissionId = !empty($decodedId) ? (int)($decodedId[0]) : null; -if ($form->workspace == null || !$form->editable_submissions || !$submissionId) { +if ($form->workspace == null || !$form->editable_submissions || $submissionId === null) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
api/app/Http/Controllers/Forms/PublicFormController.php(1 hunks)api/app/Jobs/Form/StoreFormSubmissionJob.php(4 hunks)client/composables/forms/usePartialSubmission.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- client/composables/forms/usePartialSubmission.js
- api/app/Jobs/Form/StoreFormSubmissionJob.php
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build the Nuxt app
🔇 Additional comments (1)
api/app/Http/Controllers/Forms/PublicFormController.php (1)
122-127: Clear approach for handling partial submissions.The conditional logic is clean and properly checks for:
- The
is_partialflag in the request- That the form has partial submissions enabled
- That the form is a pro feature
This maintains proper feature gating while keeping the code readable.
Done |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
api/app/Http/Controllers/Forms/PublicFormController.php (2)
96-130: Consider unifying partial and regular submissions for better maintainability.
This method duplicates parts of the submission logic from the standard flow. To reduce complexity, you could centralize the shared steps (e.g., job creation, submission ID retrieval) into a common helper or a single method that receives flags indicating whether the submission is partial or not.
155-155: Refactor the duplicated logic for encoding the submission ID.
Lines 155 and 160 both encode the submission ID after handling the job. You can unify this into a single step after the conditional block to avoid code repetition.if ($formSubmissionProcessor->shouldProcessSynchronously($form)) { $job->handle(); - $encodedSubmissionId = Hashids::encode($job->getSubmissionId()); $submissionData = $job->getProcessedData(); } else { $job->handle(); - $encodedSubmissionId = Hashids::encode($job->getSubmissionId()); } + $encodedSubmissionId = Hashids::encode($job->getSubmissionId());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
api/app/Http/Controllers/Forms/PublicFormController.php(2 hunks)client/components/open/forms/OpenCompleteForm.vue(4 hunks)client/composables/forms/pendingSubmission.js(2 hunks)client/composables/forms/usePartialSubmission.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- client/components/open/forms/OpenCompleteForm.vue
- client/composables/forms/usePartialSubmission.js
- client/composables/forms/pendingSubmission.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build the Nuxt app
🔇 Additional comments (17)
api/app/Http/Controllers/Forms/PublicFormController.php (17)
19-19: No issues with the new import.
89-95: Docblock clarifies the method intention well.
137-141: Conditional check for partial submissions is correct.
143-143: No additional commentary needed here.
146-146: No additional commentary needed here.
149-149: No additional commentary needed here.
159-160: Same duplication as above.
163-163: No additional commentary needed here.
166-166: No issues with assigning the encoded submission ID.
171-172: Docblock is clear and concise.
179-179: The method signature is straightforward.
180-181: Reading and decoding the submission hash is well-structured.
189-189: No additional commentary needed here.
191-191: No additional commentary needed here.
197-197: No additional commentary needed here.
203-203: No additional commentary needed here.
205-205: Gracefully handles invalid decoding.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
client/components/open/forms/OpenForm.vue (1)
426-428: Added submission hash for partial submissions.The
submission_hashis now properly set when partial submissions are enabled.Consider adding a comment to explain what this hash is used for and how it relates to partial submissions:
if (this.form?.enable_partial_submissions) { + // Set the submission hash to allow the backend to identify and update the correct partial submission this.dataForm.submission_hash = this.partialSubmission.getSubmissionHash() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
api/app/Http/Requests/AnswerFormRequest.php(1 hunks)api/app/Http/Requests/UserFormRequest.php(1 hunks)api/app/Jobs/Form/StoreFormSubmissionJob.php(4 hunks)api/app/Models/Forms/Form.php(3 hunks)client/components/open/forms/OpenCompleteForm.vue(4 hunks)client/components/open/forms/OpenForm.vue(6 hunks)client/components/open/tables/OpenTable.vue(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- api/app/Http/Requests/AnswerFormRequest.php
- client/components/open/forms/OpenCompleteForm.vue
- api/app/Http/Requests/UserFormRequest.php
- client/components/open/tables/OpenTable.vue
🧰 Additional context used
🧬 Code Definitions (1)
api/app/Models/Forms/Form.php (1)
api/app/Models/Forms/FormSubmission.php (1)
FormSubmission(8-36)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build the Nuxt app
🔇 Additional comments (13)
api/app/Models/Forms/Form.php (3)
95-95: Property addition looks correct.The new
enable_partial_submissionsproperty has been properly added to the fillable array, allowing it to be mass assigned.
113-114: Property cast is correctly implemented.The
enable_partial_submissionsproperty is appropriately cast as a boolean, which ensures proper type conversion when retrieving from the database.
178-178:❓ Verification inconclusive
Behavior change in submissions count - verify dependent functionality.
This change modifies the
getSubmissionsCountAttribute()to count only completed submissions instead of all submissions. While this aligns with the new partial submissions feature, it represents a significant behavior change.Make sure that any functionality dependent on
submissions_countexpects this new behavior. For example, check the following areas:
- Any admin dashboards showing submission counts
- Form status checks related to submission limits (e.g.,
max_submissions_count)- Billing or quota calculations based on submission counts
🏁 Script executed:
#!/bin/bash # Search for places where submissions_count is used echo "Places where submissions_count is used:" rg -A 3 -B 3 "submissions_count" # Check for max_submissions_count usage echo "Places where max_submissions_count is referenced:" rg -A 3 -B 3 "max_submissions_count" # Check getMaxNumberOfSubmissionsReachedAttribute implementation echo "Checking max submissions reached attribute implementation:" rg -A 5 "getMaxNumberOfSubmissionsReachedAttribute"Length of output: 29555
Action Required: Verify Dependent Functionality for Updated Submission Counting
The change in the
api/app/Models/Forms/Form.phpfile now causesgetSubmissionsCountAttribute()to count only completed submissions (usingFormSubmission::STATUS_COMPLETED). This aligns with the new partial submissions feature but represents a notable behavior change that may affect multiple parts of the codebase. In particular, please verify that:
- Admin dashboards and reporting: Components such as those in
client/pages/home.vueand similar views display submission counts as expected.- Form status and limits: The logic in
getMaxNumberOfSubmissionsReachedAttribute()(in the same Form model) and related UI elements inclient/pages/forms/[slug]/show.vuecorrectly handle the updated count when enforcingmax_submissions_count.- Integrations and tests: Usage in tests (e.g. in
api/tests/Feature/Forms/CleanDatabaseTest.phpandapi/tests/Feature/Forms/AnswerFormTest.php), as well as integrations (Discord, Slack, Email) that referencesubmissions_count, confirm that only completed submissions are expected in the new workflow.- Aggregations: Any aggregate calculations (like the Workspace model summing form submission counts) are validated to ensure they centralize around the revised definition.
Please perform a review of the listed areas to confirm the new behavior is fully compatible with dependent functionality.
client/components/open/forms/OpenForm.vue (3)
131-132: Import statements look good.The import for the new
usePartialSubmissioncomposable is correctly added, and the formatting of the existing import was standardized.
194-194: New partialSubmission state added correctly.The
partialSubmissionreactive state is properly initialized using the new composable and connected to the form and dataForm objects.
207-207: Added tracking for partial submission state.The
partialSubmissionStartedflag is used to prevent repeatedly starting the partial submission sync process.api/app/Jobs/Form/StoreFormSubmissionJob.php (7)
22-40: Well-documented class with clear explanations.Excellent job documenting the job class, its purpose, and how metadata fields are processed. The documentation clearly explains the behavior for partial submissions.
48-51: Property type improvements and new fields.Good improvements:
- Changed
submissionIdfrom?stringto?intfor better type safety- Added properties for tracking completion time and partial status
60-62: Constructor simplified to extract metadata from submission data.Good approach to simplify the constructor by removing the explicit
completionTimeparameter and instead extract it from the submission data.
71-87: Improved submission handling flow.The handle method now properly extracts metadata, processes the submission with the appropriate status, and only triggers the
FormSubmittedevent for completed submissions. This is a good separation of concerns.
98-119: Well-implemented metadata extraction.The
extractMetadatamethod is well-structured with appropriate type checking and casting:
- Properly validates that submission_id is numeric before casting to int
- Correctly extracts and removes metadata fields from the submission data
- Properly casts is_partial to boolean
138-160: Improved submission storage logic.The refactored
storeSubmissionmethod has several improvements:
- Simplified logic for finding existing or creating new submissions
- Clear setting of submission properties
- Properly setting the status based on whether it's a partial or completed submission
336-339: Ensured submission ID is included in processed data.Good improvement to ensure the submission ID is always included in the processed data returned by
getProcessedData().
| if (!this.adminPreview && this.form?.enable_partial_submissions) { | ||
| this.partialSubmission.stopSync() | ||
| } |
There was a problem hiding this comment.
Same variable naming inconsistency in beforeUnmount.
The same inconsistency between adminPreview and isAdminPreview appears here.
Apply this change to fix the inconsistency:
-if (!this.adminPreview && this.form?.enable_partial_submissions) {
+if (!this.isAdminPreview && this.form?.enable_partial_submissions) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!this.adminPreview && this.form?.enable_partial_submissions) { | |
| this.partialSubmission.stopSync() | |
| } | |
| if (!this.isAdminPreview && this.form?.enable_partial_submissions) { | |
| this.partialSubmission.stopSync() | |
| } |
| if (!this.adminPreview && this.form?.enable_partial_submissions && oldValue && Object.keys(oldValue).length > 0 && !this.partialSubmissionStarted) { | ||
| this.partialSubmission.startSync() | ||
| this.partialSubmissionStarted = true | ||
| } |
There was a problem hiding this comment.
Variable naming inconsistency in conditional check.
There's a discrepancy between the variable name used here (adminPreview) and the computed property defined in the component (isAdminPreview).
Apply this change to fix the inconsistency:
-if (!this.adminPreview && this.form?.enable_partial_submissions && oldValue && Object.keys(oldValue).length > 0 && !this.partialSubmissionStarted) {
+if (!this.isAdminPreview && this.form?.enable_partial_submissions && oldValue && Object.keys(oldValue).length > 0 && !this.partialSubmissionStarted) {Also, consider extracting this complex condition to a method to improve readability:
+methods: {
+ // Add this method
+ shouldStartPartialSubmissionSync(oldValue) {
+ return !this.isAdminPreview &&
+ this.form?.enable_partial_submissions &&
+ oldValue &&
+ Object.keys(oldValue).length > 0 &&
+ !this.partialSubmissionStarted;
+ },
+
// Existing methods...
}
// Then in the watcher:
-if (!this.adminPreview && this.form?.enable_partial_submissions && oldValue && Object.keys(oldValue).length > 0 && !this.partialSubmissionStarted) {
+if (this.shouldStartPartialSubmissionSync(oldValue)) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!this.adminPreview && this.form?.enable_partial_submissions && oldValue && Object.keys(oldValue).length > 0 && !this.partialSubmissionStarted) { | |
| this.partialSubmission.startSync() | |
| this.partialSubmissionStarted = true | |
| } | |
| <script> | |
| export default { | |
| // ... other component options such as data, computed, etc. | |
| computed: { | |
| isAdminPreview() { | |
| // computed logic returning whether the current view is an admin preview | |
| } | |
| }, | |
| methods: { | |
| shouldStartPartialSubmissionSync(oldValue) { | |
| return !this.isAdminPreview && | |
| this.form?.enable_partial_submissions && | |
| oldValue && | |
| Object.keys(oldValue).length > 0 && | |
| !this.partialSubmissionStarted; | |
| }, | |
| // ... existing methods | |
| }, | |
| watch: { | |
| // Assuming this watcher is monitoring changes where the following snippet is used: | |
| someWatcher(oldValue) { | |
| if (this.shouldStartPartialSubmissionSync(oldValue)) { | |
| this.partialSubmission.startSync() | |
| this.partialSubmissionStarted = true | |
| } | |
| } | |
| }, | |
| // ... other component options | |
| } | |
| </script> |
There was a problem hiding this comment.
Great progress.
Please add some tests.
Examples:
- Test Edge Cases: Ensure automated tests cover scenarios like:
- Submitting partially, then completing using the returned hash.
- Submitting partially multiple times for the same submission.
- Attempting partial submission when the feature is disabled for the form.
- Correct stats calculation for partial vs. completed submissions.
- File/signature handling during partial saves.
Check database feature clash
Also there's a feature clash with "does not exist in submission" or "exisits in submisison". we when checking for these, we need to filter out pending submission otherwise pending submission conflicts with itself.
- Updated the docblock for the method responsible for processing submission identifiers to clarify its functionality. The method now explicitly states that it converts a submission hash or string ID into a numeric submission_id, ensuring consistent internal storage format. These changes aim to improve code documentation and enhance understanding of the method's purpose and behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
api/app/Http/Controllers/Forms/PublicFormController.php (4)
89-130: Solid approach for partial submissions, with one improvement needed.This private method correctly persists partial submissions and checks that at least one field has a value. However, consider these points:
- Check for existing partial submissions: If the user already has an unfinalized partial submission, you might consolidate or update it rather than always creating a new record. Otherwise, repeated page refreshes or “fill again” actions could lead to multiple partials.
- Refactor for clarity: This method is relatively long. You might improve readability by extracting the "at least one non-empty field" logic into a helper method.
-private function handlePartialSubmissions(Request $request) +private function handlePartialSubmission(Request $request) { // ... }
137-141: Validate scenarios whereis_partialis set but not allowed by the form.Currently, you silently bypass partial submissions if
$form->enable_partial_submissionsor$form->is_prois false. Consider returning an error or ignoringis_partialin such cases to avoid confusion on the client side.
155-155: Minimal duplication with line 160.You encode the submission ID here and again in line 160. Consider extracting that logic once outside the conditional, if feasible, to reduce repetition.
159-160: Dispatch vs. handle.Here, and in line 121, you are calling
$job->handle()directly. If you intend to queue these submissions asynchronously, you might opt for$this->dispatch($job)or$job->dispatch()to avoid synchronous blocking.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/app/Http/Controllers/Forms/PublicFormController.php(2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
api/app/Http/Controllers/Forms/PublicFormController.php (4)
api/app/Models/Forms/FormSubmission.php (1)
form(32-35)api/app/Jobs/Form/StoreFormSubmissionJob.php (4)
StoreFormSubmissionJob(41-341)handle(69-88)getSubmissionId(126-129)getProcessedData(329-340)api/app/Http/Requests/AnswerFormRequest.php (1)
AnswerFormRequest(20-312)api/app/Service/Forms/FormSubmissionProcessor.php (3)
FormSubmissionProcessor(8-79)shouldProcessSynchronously(13-33)getRedirectData(58-78)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build the Nuxt app
🔇 Additional comments (8)
api/app/Http/Controllers/Forms/PublicFormController.php (8)
143-143: No concerns here.This line merely documents retrieving validated data.
146-146: No issues identified.The comment clarifies that submission hash and ID are being processed next.
149-149: Clean approach to job creation.Instantiating the
StoreFormSubmissionJobwith all data keeps the flow consistent for both partial and full submissions.
163-163: No issues.This comment line simply marks the response return section.
166-166: Good to expose encoded submission ID.Returning the encoded ID aligns with the rest of the partial submissions logic.
171-203: Robust solution for decoding identifiers, but missing ownership check.This method diligently decodes hashed IDs before storing them. However, similar to prior recommendations:
- Consider verifying that the decoded ID truly belongs to
$formbefore using it. Failing to do so might allow referencing submissions from other forms.- Graceful fallback: If invalid, you can either nullify
submission_idor abort with an error message.(This security check echoes a similar concern noted in a previous comment.)
207-210: Decoding logic aligns withprocessSubmissionIdentifiers.The fallback to
falseif decoding fails is a good practice. The subsequent checks (lines 228-232) correctly disallow access to mismatched forms.
19-19:❓ Verification inconclusive
Great import usage.
The
Strfacade is necessary for UUID checks on submission fields. Ensure your Laravel version includes theStr::isUuidmethod (introduced in Laravel 9).Here’s a quick script to confirm that your application's Laravel version supports
Str::isUuid:
🏁 Script executed:
#!/bin/bash # Description: Check Laravel version in composer.json and relevant class references rg '"laravel/framework":' composer.json -A 2 rg -A 3 'isUuid' vendor/laravel/framework/src/Illuminate/Support/Str.phpLength of output: 269
Attention: Verify Laravel Environment for
Str::isUuidThe import of
Illuminate\Support\Strinapi/app/Http/Controllers/Forms/PublicFormController.phpis correctly used for UUID validation. However, our automated check couldn’t locate the expectedcomposer.jsonor vendor files (e.g. thevendor/laravel/framework/src/Illuminate/Support/Str.phpfile) to confirm that your Laravel version supportsStr::isUuid(introduced in Laravel 9).
- Repository Setup: Ensure that your project’s root contains a valid
composer.jsonand that the vendor directory is properly populated.- Laravel Version Check: Manually verify that the Laravel framework version in use is 9 or higher so that the
Str::isUuidmethod is available.No changes to the import itself are needed provided your environment meets these prerequisites.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
api/app/Jobs/Form/StoreFormSubmissionJob.php (2)
90-119: Well-implemented metadata extraction.The
extractMetadatamethod cleanly extracts and processes the metadata fields from the submission data. The numeric validation forsubmission_idis a good security measure to prevent type-related issues.However, consider adding a check to verify that the submission belongs to the current user to prevent potential unauthorized access to others' submissions.
if (isset($this->submissionData['submission_id']) && $this->submissionData['submission_id']) { if (is_numeric($this->submissionData['submission_id'])) { $this->submissionId = (int)$this->submissionData['submission_id']; + // Verify the submission belongs to the current form to prevent unauthorized access + if (!$this->form->submissions()->where('id', $this->submissionId)->exists()) { + $this->submissionId = null; + Log::warning('Attempted to access submission that does not belong to the current form', [ + 'form_id' => $this->form->id, + 'submission_id' => $this->submissionId + ]); + } } unset($this->submissionData['submission_id']); }
136-160: Improved submission storage logic.The refactored
storeSubmissionmethod now properly handles both the creation of new submissions and updating of existing ones. The status setting based on theisPartialflag directly addresses the issue mentioned in the PR objectives about duplicate submissions when refreshing the page.Consider adding error handling for the case where
findOrFailmight throw an exception if the submission ID doesn't exist:// Find existing submission or create a new one -$submission = $this->submissionId - ? $this->form->submissions()->findOrFail($this->submissionId) - : new FormSubmission(); +$submission = null; +if ($this->submissionId) { + try { + $submission = $this->form->submissions()->findOrFail($this->submissionId); + } catch (\Illuminate\Database\Eloquent\ModelNotFoundException $e) { + Log::warning('Attempted to update non-existent submission', [ + 'form_id' => $this->form->id, + 'submission_id' => $this->submissionId + ]); + $this->submissionId = null; + } +} + +if (!$submission) { + $submission = new FormSubmission(); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/app/Jobs/Form/StoreFormSubmissionJob.php(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build the Nuxt app
🔇 Additional comments (9)
api/app/Jobs/Form/StoreFormSubmissionJob.php (9)
20-21: Appropriate import addition for logging.The addition of the Log facade is correctly imported to support the new debugging statements added later in the code.
22-40: Well-documented class purpose and behavior.The detailed class documentation clearly explains the purpose of the job, the metadata fields it processes, and how partial submissions are handled. This is excellent for maintainability and understanding the code's behavior.
48-51: Appropriate property type changes and additions.The change from
?stringto?intfor$submissionIdenforces proper typing. The new private properties for tracking completion time and partial submission status directly support the PR's objective of handling partial submissions.
56-60: Simplified constructor with improved documentation.The constructor has been simplified by removing the
$completionTimeparameter, as this value is now extracted from the submission data. The parameter documentation has been improved to clarify that submission data includes metadata fields.
71-87: Improved handling logic with partial submission support.The modifications correctly implement the extraction of metadata and conditional event dispatching based on whether the submission is partial. This directly addresses the issue mentioned in the PR objectives about handling existing submissions properly.
121-129: Simple and effective getter method.The
getSubmissionIdmethod is a straightforward implementation that provides access to the submission ID.
270-275: Added helpful debugging for file operations.The addition of debug logging when moving files to permanent storage will help troubleshoot file upload issues, which were mentioned in the PR objectives.
324-340: Ensures submission ID is included in returned data.The updated
getProcessedDatamethod now ensures that the submission ID is included in the returned data, which is essential for client-side tracking of partial submissions.
41-342:❓ Verification inconclusive
Verify edge cases for the partial submissions flow.
The implementation of partial submissions looks well designed, but it's important to verify the behavior when handling edge cases such as:
- Users submitting a partial form, then trying to edit it after it's expired
- Multiple partial submissions for the same form
- Concurrent updates to the same partial submission
🏁 Script executed:
#!/bin/bash # Let's check how many forms are currently configured to use partial submissions # and if there are any existing validation or handling mechanisms # Check for forms with auto_save enabled echo "Checking for forms with auto_save enabled:" rg -i "auto_save.*true" --type php # Check for specific handling of partial submissions in controllers echo "Checking for partial submission handling in controllers:" rg -A 5 "STATUS_PARTIAL" --type php # Look for any tests related to partial submissions echo "Checking for tests related to partial submissions:" rg -A 3 "test.*partial.*submission" --type phpLength of output: 4000
Action Required: Verify Edge Cases for Partial Submissions Flow
The partial submissions handling appears to be implemented correctly—using the designated
STATUS_PARTIALflag and ensuring that completed submissions trigger integrations. However, please ensure that the following edge cases are thoroughly tested and confirmed in your environment:
- Expired Partial Submissions: Verify that when a user attempts to edit an expired partial submission, the system gracefully handles the update or prevents it as expected.
- Multiple Partial Submissions: Confirm that if multiple partial submissions occur for the same form, the system distinguishes between them appropriately and avoids data conflicts.
- Concurrent Updates: Ensure that concurrent updates to the same partial submission do not lead to race conditions or data inconsistency.
Given that there is limited automated test coverage for these scenarios (as indicated by the absence of dedicated tests in the codebase), please validate these behaviors manually or consider adding targeted tests.
- Updated the query in FormLogicConditionChecker to exclude submissions with a status of 'partial', ensuring that only complete submissions are processed. - Minor formatting adjustment in the docblock of PublicFormController for improved clarity. These changes aim to refine submission handling and enhance the accuracy of form logic evaluations.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
api/tests/Feature/Submissions/PartialSubmissionTest.php (5)
8-47: Test case doesn't verify partial submission status after initial submission.The test correctly verifies that a partial submission can be completed later, but it doesn't check if the submission is properly marked with a partial status after the initial submission. This verification would ensure the submission is correctly stored in a partial state before completing it.
Add a verification after the initial submission:
$submissionHash = $partialResponse->json('submission_hash'); expect($submissionHash)->not->toBeEmpty(); + // Verify partial submission state + $partialSubmission = FormSubmission::first(); + expect($partialSubmission->status)->toBe(FormSubmission::STATUS_PARTIAL); // Complete the submission using the hash
103-138: Enhance file upload test to cover updating files in partial submissions.The current test verifies that uploaded files are preserved when completing a partial submission, but doesn't test replacing or updating files in subsequent partial submissions.
Extend the test to cover updating files:
$response = $this->postJson(route('forms.answer', $form->slug), $formData) ->assertSuccessful(); $submissionHash = $response->json('submission_hash'); + // Update the partial submission with a different file + $updatedFile = UploadedFile::fake()->create('updated.pdf', 200); + $updateData = $this->generateFormSubmissionData($form); + $updateData[$fileFieldId] = $updatedFile; + $updateData['is_partial'] = true; + $updateData['submission_hash'] = $submissionHash; + + $this->postJson(route('forms.answer', $form->slug), $updateData) + ->assertSuccessful(); + + // Verify file was updated + $partialSubmission = FormSubmission::first(); + expect($partialSubmission->status)->toBe(FormSubmission::STATUS_PARTIAL); + expect($partialSubmission->data)->toHaveKey($fileFieldId); + $updatedFilePath = str_replace('storage/', '', $partialSubmission->data[$fileFieldId]); + expect(Storage::disk('local')->exists($updatedFilePath))->toBeTrue(); + // Complete the submission
140-172: Enhance signature test to cover updating signatures in partial submissions.Similar to the file upload test, the signature test verifies preservation but doesn't test updating the signature in subsequent partial submissions.
Extend the test to cover updating signatures:
$response = $this->postJson(route('forms.answer', $form->slug), $formData) ->assertSuccessful(); $submissionHash = $response->json('submission_hash'); + // Update the partial submission with a different signature + $updatedData = $this->generateFormSubmissionData($form); + $updatedData[$signatureFieldId] = 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUg...'; // Different signature data + $updatedData['is_partial'] = true; + $updatedData['submission_hash'] = $submissionHash; + + $this->postJson(route('forms.answer', $form->slug), $updatedData) + ->assertSuccessful(); + + // Verify signature was updated + $partialSubmission = FormSubmission::first(); + expect($partialSubmission->status)->toBe(FormSubmission::STATUS_PARTIAL); + // Complete the submission
8-47: Add test for invalid submission hash handling.There's no test for how the system handles invalid or non-existent submission hashes. This is an important edge case to cover.
Add a test case for invalid submission hash:
it('handles invalid submission hash gracefully', function () { $user = $this->actingAsProUser(); $workspace = $this->createUserWorkspace($user); $form = $this->createForm($user, $workspace, [ 'enable_partial_submissions' => true ]); // Submit with invalid hash $formData = $this->generateFormSubmissionData($form, ['text' => 'Test']); $formData['submission_hash'] = 'invalid-hash-123'; $response = $this->postJson(route('forms.answer', $form->slug), $formData); // Verify appropriate error response $response->assertStatus(422) ->assertJsonStructure(['message']); // Alternate acceptable behavior: creates new submission if hash invalid // $response->assertSuccessful(); // expect(FormSubmission::count())->toBe(1); });
1-238: Consider adding a test for handling stale partial submissions.There are no tests for handling stale or abandoned partial submissions. Depending on your requirements, you might want to test automatic cleanup of old partial submissions or any TTL-related functionality.
If your system includes cleanup of stale submissions, consider adding a test:
it('cleans up stale partial submissions', function () { $user = $this->actingAsProUser(); $workspace = $this->createUserWorkspace($user); $form = $this->createForm($user, $workspace, [ 'enable_partial_submissions' => true, 'partial_submission_ttl' => 60 // TTL in minutes ]); // Create a partial submission $formData = $this->generateFormSubmissionData($form, ['text' => 'Test']); $formData['is_partial'] = true; $this->postJson(route('forms.answer', $form->slug), $formData) ->assertSuccessful(); // Verify submission exists expect(FormSubmission::count())->toBe(1); // Simulate time passing $submission = FormSubmission::first(); $submission->updated_at = now()->subMinutes(120); // 2 hours ago $submission->save(); // Trigger cleanup (assuming you have a cleanup command/job) $this->artisan('submissions:cleanup-stale'); // Verify stale submission was removed expect(FormSubmission::count())->toBe(0); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/tests/Feature/Submissions/PartialSubmissionTest.php(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build the Nuxt app
🔇 Additional comments (2)
api/tests/Feature/Submissions/PartialSubmissionTest.php (2)
192-213: Test coverage looks good for this section.The test properly verifies that when partial submissions are disabled, all submissions are treated as complete, even when specifically marked as partial. This ensures the feature flag is respected.
215-237: Test coverage looks good for non-pro users.This test correctly validates that for non-pro users, the partial submission feature is unavailable and all submissions are treated as complete. This ensures proper feature restriction based on user type.
| it('requires at least one field with value for partial submission', function () { | ||
| $user = $this->actingAsProUser(); | ||
| $workspace = $this->createUserWorkspace($user); | ||
| $form = $this->createForm($user, $workspace, [ | ||
| 'enable_partial_submissions' => true | ||
| ]); | ||
|
|
||
| // Try to submit with empty data | ||
| $formData = ['is_partial' => true]; | ||
|
|
||
| $this->postJson(route('forms.answer', $form->slug), $formData) | ||
| ->assertStatus(422) | ||
| ->assertJson([ | ||
| 'message' => 'At least one field must have a value for partial submissions.' | ||
| ]); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Good validation test, but missing specific "fill again" scenario.
This test verifies that empty partial submissions are rejected, which is good. However, it doesn't explicitly test the "fill again" functionality mentioned in the PR objectives, where clicking "fill again" creates a pending submission even when the form is empty.
Consider adding a specific test that simulates the "fill again" scenario to ensure it correctly handles empty forms:
it('does not create empty partial submission when using fill again', function () {
$user = $this->actingAsProUser();
$workspace = $this->createUserWorkspace($user);
$form = $this->createForm($user, $workspace, [
'enable_partial_submissions' => true
]);
// Simulate "fill again" action with empty form data
$formData = ['is_partial' => true, 'fill_again' => true];
$this->postJson(route('forms.answer', $form->slug), $formData)
->assertStatus(422)
->assertJson([
'message' => 'At least one field must have a value for partial submissions.'
]);
// Verify no submission was created
expect(FormSubmission::count())->toBe(0);
});| it('can submit form partially and complete it later using submission hash', function () { | ||
| $user = $this->actingAsProUser(); | ||
| $workspace = $this->createUserWorkspace($user); | ||
| $form = $this->createForm($user, $workspace, [ | ||
| 'enable_partial_submissions' => true | ||
| ]); | ||
|
|
||
| // Initial partial submission | ||
| $formData = $this->generateFormSubmissionData($form, ['text' => 'Initial Text']); | ||
| $formData['is_partial'] = true; | ||
|
|
||
| $partialResponse = $this->postJson(route('forms.answer', $form->slug), $formData) | ||
| ->assertSuccessful() | ||
| ->assertJson([ | ||
| 'type' => 'success', | ||
| 'message' => 'Partial submission saved', | ||
| ]); | ||
|
|
||
| $submissionHash = $partialResponse->json('submission_hash'); | ||
| expect($submissionHash)->not->toBeEmpty(); | ||
|
|
||
| // Complete the submission using the hash | ||
| $completeData = $this->generateFormSubmissionData($form, [ | ||
| 'text' => 'Complete Text', | ||
| 'email' => '[email protected]' | ||
| ]); | ||
| $completeData['submission_hash'] = $submissionHash; | ||
|
|
||
| $this->postJson(route('forms.answer', $form->slug), $completeData) | ||
| ->assertSuccessful() | ||
| ->assertJson([ | ||
| 'type' => 'success', | ||
| 'message' => 'Form submission saved.', | ||
| ]); | ||
|
|
||
| // Verify final submission state | ||
| $submission = FormSubmission::first(); | ||
| expect($submission->status)->toBe(FormSubmission::STATUS_COMPLETED); | ||
| expect($submission->data)->toHaveKey(array_key_first($completeData)); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test for the page refresh duplicate submission issue.
The PR objectives mention an issue where refreshing the page and submitting creates a new record instead of finalizing the pending submission. This scenario isn't explicitly tested.
Add a test case that simulates the page refresh scenario:
it('does not create duplicate submissions when refreshing page', function () {
$user = $this->actingAsProUser();
$workspace = $this->createUserWorkspace($user);
$form = $this->createForm($user, $workspace, [
'enable_partial_submissions' => true
]);
// Initial partial submission
$formData = $this->generateFormSubmissionData($form, ['text' => 'Initial Text']);
$formData['is_partial'] = true;
$partialResponse = $this->postJson(route('forms.answer', $form->slug), $formData)
->assertSuccessful();
$submissionHash = $partialResponse->json('submission_hash');
// Simulate page refresh by making the same request without hash
$this->postJson(route('forms.answer', $form->slug), $formData)
->assertSuccessful();
// Verify only one submission exists
expect(FormSubmission::count())->toBe(1);
// Complete the submission and verify again
$completeData = $this->generateFormSubmissionData($form, ['text' => 'Complete Text']);
$completeData['submission_hash'] = $submissionHash;
$this->postJson(route('forms.answer', $form->slug), $completeData)
->assertSuccessful();
expect(FormSubmission::count())->toBe(1);
});…stency - Updated the `FormSubmissionController` to improve code consistency by adjusting the formatting of anonymous functions in the `filter` and `first` methods. - Modified `PartialSubmissionTest` to simplify the `Storage::fake()` method call, removing the unnecessary 'local' parameter for better clarity. These changes aim to enhance code readability and maintainability across the form submission handling and testing components.
JhumanJ
force-pushed
the
627a0-partial-submissions
branch
from
d3b03e3 to
c68333a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
api/tests/Feature/Submissions/PartialSubmissionTest.php (4)
8-47: 🛠️ Refactor suggestionGood test for basic partial submission workflow, but missing page refresh scenario.
This test effectively validates the core functionality of saving a partial submission and completing it later using a submission hash. However, it doesn't address the page refresh issue mentioned in the PR objectives, where refreshing the page and submitting creates a new record instead of finalizing the pending submission.
174-189: 🛠️ Refactor suggestionGood validation test, but missing specific "fill again" scenario.
This test verifies that empty partial submissions are rejected, which is good. However, it doesn't explicitly test the "fill again" functionality mentioned in the PR objectives, where clicking "fill again" creates a pending submission even when the form is empty.
1-237: 🛠️ Refactor suggestionMissing test for duplicate submission prevention during page refresh.
The PR objectives specifically mention an issue where refreshing the page and then submitting creates a new record instead of finalizing the pending one. The test suite doesn't include a specific test to verify this issue is fixed.
+it('does not create duplicate submissions when refreshing page', function () { + $user = $this->actingAsProUser(); + $workspace = $this->createUserWorkspace($user); + $form = $this->createForm($user, $workspace, [ + 'enable_partial_submissions' => true + ]); + + // Initial partial submission + $formData = $this->generateFormSubmissionData($form, ['text' => 'Initial Text']); + $formData['is_partial'] = true; + + $partialResponse = $this->postJson(route('forms.answer', $form->slug), $formData) + ->assertSuccessful(); + $submissionHash = $partialResponse->json('submission_hash'); + + // Simulate page refresh by making the same request without hash + $this->postJson(route('forms.answer', $form->slug), $formData) + ->assertSuccessful(); + + // Verify only one submission exists + expect(FormSubmission::count())->toBe(1); + + // Complete the submission and verify again + $completeData = $this->generateFormSubmissionData($form, ['text' => 'Complete Text']); + $completeData['submission_hash'] = $submissionHash; + + $this->postJson(route('forms.answer', $form->slug), $completeData) + ->assertSuccessful(); + + expect(FormSubmission::count())->toBe(1); +});
1-237: 🛠️ Refactor suggestionMissing test for "fill again" functionality.
The PR objectives mention an issue where clicking "fill again" creates a pending submission even when the form is empty. The test suite should include a test to verify this issue is fixed.
+it('does not create empty partial submission when using fill again', function () { + $user = $this->actingAsProUser(); + $workspace = $this->createUserWorkspace($user); + $form = $this->createForm($user, $workspace, [ + 'enable_partial_submissions' => true + ]); + + // Simulate "fill again" action with empty form data + $formData = ['is_partial' => true, 'fill_again' => true]; + + $this->postJson(route('forms.answer', $form->slug), $formData) + ->assertStatus(422) + ->assertJson([ + 'message' => 'At least one field must have a value for partial submissions.' + ]); + + // Verify no submission was created + expect(FormSubmission::count())->toBe(0); +});
🧹 Nitpick comments (2)
api/tests/Feature/Submissions/PartialSubmissionTest.php (2)
103-138: File upload test covers the happy path but not edge cases.While this test verifies that files uploaded in a partial submission are preserved when completed, it doesn't address the specific file upload issues mentioned in the PR objectives. Consider adding tests for problematic scenarios like refreshing the page after uploading a file.
// Add an additional test that simulates page refresh after file upload +it('preserves file uploads when page is refreshed during partial submission', function () { + Storage::fake(); + + $user = $this->actingAsProUser(); + $workspace = $this->createUserWorkspace($user); + $form = $this->createForm($user, $workspace, [ + 'enable_partial_submissions' => true + ]); + + // Create a fake file + $file = UploadedFile::fake()->create('test.pdf', 100); + + // First partial submission with file + $formData = $this->generateFormSubmissionData($form); + $fileFieldId = collect($form->properties)->where('type', 'files')->first()['id']; + $formData[$fileFieldId] = $file; + $formData['is_partial'] = true; + + $response = $this->postJson(route('forms.answer', $form->slug), $formData) + ->assertSuccessful(); + + $submissionHash = $response->json('submission_hash'); + + // Simulate page refresh by submitting again without hash + $refreshFormData = $this->generateFormSubmissionData($form); + $refreshFormData[$fileFieldId] = $file; + $refreshFormData['is_partial'] = true; + + $this->postJson(route('forms.answer', $form->slug), $refreshFormData) + ->assertSuccessful(); + + // Verify no duplicate submission was created + expect(FormSubmission::count())->toBe(1); +});
140-172: Signature field test covers the happy path but not edge cases.The test verifies that signatures in a partial submission are preserved when completed, but doesn't address the specific signature issues mentioned in the PR objectives. Consider adding tests that tackle the problematic scenarios mentioned.
// Add an additional test that simulates page refresh after adding a signature +it('preserves signature when page is refreshed during partial submission', function () { + Storage::fake(); + + $user = $this->actingAsProUser(); + $workspace = $this->createUserWorkspace($user); + $form = $this->createForm($user, $workspace, [ + 'enable_partial_submissions' => true + ]); + + // Create partial submission with signature + $formData = $this->generateFormSubmissionData($form); + $signatureFieldId = collect($form->properties)->where('type', 'files')->first()['id']; + $formData[$signatureFieldId] = 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAA...'; // Base64 signature data + $formData['is_partial'] = true; + + $response = $this->postJson(route('forms.answer', $form->slug), $formData) + ->assertSuccessful(); + + // Simulate page refresh by submitting without hash + $refreshFormData = $this->generateFormSubmissionData($form); + $refreshFormData[$signatureFieldId] = 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAA...'; // Base64 signature data + $refreshFormData['is_partial'] = true; + + $this->postJson(route('forms.answer', $form->slug), $refreshFormData) + ->assertSuccessful(); + + // Verify no duplicate submission was created + expect(FormSubmission::count())->toBe(1); +});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
api/app/Http/Controllers/Forms/FormSubmissionController.php(3 hunks)api/tests/Feature/Submissions/PartialSubmissionTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/app/Http/Controllers/Forms/FormSubmissionController.php
🧰 Additional context used
🧬 Code Definitions (1)
api/tests/Feature/Submissions/PartialSubmissionTest.php (1)
api/app/Http/Controllers/Forms/FormSubmissionController.php (1)
submissions(27-33)
🔇 Additional comments (4)
api/tests/Feature/Submissions/PartialSubmissionTest.php (4)
49-79: Good test for multiple updates to partial submissions.This test effectively verifies that partial submissions can be updated multiple times while maintaining their partial status. The test confirms that the submission data is properly updated with each submission.
81-101: Good test for partial vs completed submission statistics.This test properly validates that the system correctly tracks and differentiates between partial and completed submissions in the statistics.
191-213: Good test for when partial submissions are disabled.This test correctly verifies that submissions are saved as completed when the partial submissions feature is disabled, even if the request specifies it as partial.
215-237: Good test for non-pro forms.This test correctly verifies that submissions on non-pro forms are saved as completed even when marked as partial, ensuring the feature is properly limited to pro users only.
- Added validation to the `FormSubmissionController` by introducing `$submissionData = $request->validated();` to ensure that only validated data is processed for form submissions. - Improved code readability in the `FormSubmissionController` by adjusting the formatting of anonymous functions in the `filter` and `first` methods. - Removed unnecessary blank lines in the `EditSubmissionTest` to streamline the test setup. These changes aim to enhance data integrity during form submissions and improve overall code clarity and maintainability.
Done |
Summary by CodeRabbit
New Features
Bug Fixes
Documentation