file upload max size feature #328
Conversation
WalkthroughThe updates aim to enhance file size validation throughout the application. A new method for dynamically determining maximum file sizes has been introduced, alongside validation rules to enforce these limits. These changes cover both server-side and client-side components, ensuring consistent application of file size restrictions based on various settings like field properties, form configurations, and workspace constraints. This comprehensive approach improves user experience and system robustness by preventing oversized file uploads. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (4)
- app/Http/Requests/AnswerFormRequest.php (2 hunks)
- app/Http/Requests/UserFormRequest.php (1 hunks)
- client/components/open/forms/OpenFormField.vue (1 hunks)
- client/components/open/forms/fields/components/FieldOptions.vue (4 hunks)
Additional comments: 4
app/Http/Requests/AnswerFormRequest.php (2)
- 30-34: The implementation of the
getFieldMaxFileSizemethod is sound and aligns well with the PR's objectives of dynamically calculating file size limits. It correctly calculates the maximum file size by considering both the field-specific limit and the workspace limit, ensuring flexibility and consistency in file size validation.- 189-189: The updated usage of
getFieldMaxFileSizeingetPropertyRulesfor file validation is correctly implemented. It ensures that the file size validation dynamically considers both the field-specific and workspace limits, enhancing the robustness of file upload handling.client/components/open/forms/OpenFormField.vue (1)
- 306-306: The modification to the calculation of the
mbLimitproperty is well-implemented and aligns with the PR's objectives of dynamically managing file size limits. The use of conditional expressions and logical operators ensures that the most restrictive file size limit is applied, enhancing the application's usability and preventing user confusion.client/components/open/forms/fields/components/FieldOptions.vue (1)
- 455-457: The
mbLimitcomputed property effectively uses a fallback mechanism to determine the maximum file size, prioritizing the form'smax_file_sizeover the workspace's. This is a solid approach for flexibility and ensuring that the most specific limit is applied. However, it's important to ensure that bothform.max_file_sizeandcurrentWorkspace.max_file_sizeare always defined and validated on the backend to prevent any unexpected behavior.
| <text-input name="max_file_size" class="mt-3" :form="field" native-type="number" | ||
| :min="1" | ||
| :max="mbLimit" | ||
| label="Maximum file size" :placeholder="`1MB - ${mbLimit}MB`" | ||
| help="Set the maximum file size that can be uploaded in MB." | ||
| /> |
There was a problem hiding this comment.
The addition of the max_file_size text input field with dynamic minimum and maximum values is a good implementation for allowing users to specify file size limits. However, consider adding client-side validation to ensure that the input is a positive integer and within a reasonable range to prevent potential issues with extremely high values that could affect the system's performance or usability.
+ <v-text-field type="number" v-model.number="field.max_file_size" :min="1" :max="mbLimit" integer-only />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.
| <text-input name="max_file_size" class="mt-3" :form="field" native-type="number" | |
| :min="1" | |
| :max="mbLimit" | |
| label="Maximum file size" :placeholder="`1MB - ${mbLimit}MB`" | |
| help="Set the maximum file size that can be uploaded in MB." | |
| /> | |
| <text-input name="max_file_size" class="mt-3" :form="field" native-type="number" | |
| :min="1" | |
| :max="mbLimit" | |
| label="Maximum file size" :placeholder="`1MB - ${mbLimit}MB`" | |
| help="Set the maximum file size that can be uploaded in MB." | |
| /> | |
| <v-text-field type="number" v-model.number="field.max_file_size" :min="1" :max="mbLimit" integer-only /> |
| @@ -118,6 +118,9 @@ public function rules() | |||
| 'properties.*.generates_uuid' => 'boolean|nullable', | |||
| 'properties.*.generates_auto_increment_id' => 'boolean|nullable', | |||
|
|
|||
| // For file (min and max) | |||
| 'properties.*.max_file_size' => 'min:1|numeric', | |||
There was a problem hiding this comment.
The validation rule for max_file_size correctly ensures that the value is numeric and at least 1. However, to prevent potential abuse or unintentional configuration errors, it might be beneficial to also enforce an upper limit. Consider modifying the validation rule to include a reasonable maximum value, such as numeric|min:1|max:100 (assuming the maximum file size is 100MB as an example). This ensures that file sizes are within a reasonable range.
- 'properties.*.max_file_size' => 'min:1|numeric',
+ 'properties.*.max_file_size' => 'numeric|min:1|max:100', // Adjust the max value as appropriateCommittable 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.
| 'properties.*.max_file_size' => 'min:1|numeric', | |
| 'properties.*.max_file_size' => 'numeric|min:1|max:100', // Adjust the max value as appropriate |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- client/components/open/forms/fields/components/FieldOptions.vue (4 hunks)
Additional comments: 3
client/components/open/forms/fields/components/FieldOptions.vue (3)
- 64-69: The addition of the
<text-input>component for setting the maximum file size is well-implemented. It uses thembLimitcomputed property to dynamically set the range of acceptable file sizes, enhancing the flexibility of file upload constraints. This change aligns with the PR objectives to provide dynamic feedback on file size limits.- 438-439: The
setup()function correctly retrieves thecurrentWorkspaceusing a computed property. This approach is efficient for accessing reactive state within the Vue Composition API, ensuring that the component can dynamically adjust to changes in the workspace settings.- 455-457: The
mbLimitcomputed property effectively determines the maximum file size based on form and workspace settings. This dynamic calculation supports the PR's goal of flexible file upload constraints. It's a good practice to use computed properties for reactive data that depends on other reactive sources.
| if (this.field.type == 'files') { | ||
| this.field.max_file_size = Math.min((this.field.max_file_size ?? this.mbLimit), this.mbLimit) | ||
| } |
There was a problem hiding this comment.
The logic to set max_file_size for file type fields based on the minimum of form and workspace limits is a crucial update. It ensures that the file size limit is consistently enforced across different levels of settings. However, as previously commented, consider adding a user notification or confirmation step if the max_file_size is adjusted to a lower value than a previously set one. This would enhance user awareness and control over the changes.
+ if (this.field.max_file_size > this.mbLimit) {
+ // Notify the user that the max_file_size has been adjusted to the mbLimit
+ }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.
| if (this.field.type == 'files') { | |
| this.field.max_file_size = Math.min((this.field.max_file_size ?? this.mbLimit), this.mbLimit) | |
| } | |
| if (this.field.type == 'files') { | |
| this.field.max_file_size = Math.min((this.field.max_file_size ?? this.mbLimit), this.mbLimit) | |
| if (this.field.max_file_size > this.mbLimit) { | |
| // Notify the user that the max_file_size has been adjusted to the mbLimit | |
| } | |
| } |
Summary by CodeRabbit