Better Form Stats #567
Better Form Stats #567
Conversation
|
Warning Rate limit exceeded@JhumanJ has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 24 minutes and 10 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe pull request enhances the handling of form statistics by implementing validation for date ranges and introducing a new method for detailed statistics in the backend. A new request class ensures structured validation, while the frontend introduces a timer component to track form completion times, providing valuable analytics data. Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
client/components/open/forms/FormTimer.vue (1)
1-57: Consider minor refactorings to improve code readability and simplify the logic.Here are a few suggestions to improve the code:
- Extract the
setIntervalcallback into a separate function for better readability.- Simplify the
completionTimewatcher by directly callingpendingSubmission.setTimer.- Update the
resetTimermethod to directly setcompletionTime.valuetonullinstead of relying on the watcher.Apply this diff to implement the suggested refactorings:
-watch(() => completionTime.value, () => { - if (completionTime.value) { - pendingSubmission.setTimer(completionTime.value) - } -}, { immediate: true }) +watch(() => completionTime.value, (newValue) => { + pendingSubmission.setTimer(newValue) +}, { immediate: true }) +const incrementCompletionTime = () => { + completionTime.value += 1 +} + const startTimer = () => { if (!startTime.value) { startTime.value = parseInt(pendingSubmission.getTimer() ?? 1) completionTime.value = startTime.value - timer = setInterval(() => { - completionTime.value += 1 - }, 1000) + timer = setInterval(incrementCompletionTime, 1000) } } const resetTimer = () => { stopTimer() startTime.value = null - completionTime.value = null + completionTime.value = null }client/pages/forms/[slug]/show/stats.vue (1)
3-169: The new card layout for displaying form analytics looks great!The consistent structure of the cards with loading indicators, actual values for pro users, and blurred placeholders for non-pro users provides a clear and user-friendly presentation of the analytics data.
I noticed a commented-out section (lines 94-169) with a similar card layout using the
UCardcomponent. Is this an alternative implementation that you were considering? If it's no longer needed, consider removing it to keep the codebase clean.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (13)
- api/app/Http/Controllers/Forms/FormStatsController.php (3 hunks)
- api/app/Http/Controllers/Forms/PublicFormController.php (2 hunks)
- api/app/Http/Resources/FormSubmissionResource.php (1 hunks)
- api/app/Jobs/Form/StoreFormSubmissionJob.php (2 hunks)
- api/app/Models/Forms/FormSubmission.php (1 hunks)
- api/database/migrations/2024_09_09_180618_add_completion_time_to_form_submissions.php (1 hunks)
- api/routes/api.php (1 hunks)
- api/tests/Feature/Forms/FormStatTest.php (1 hunks)
- client/components/open/forms/FormTimer.vue (1 hunks)
- client/components/open/forms/OpenCompleteForm.vue (7 hunks)
- client/components/open/forms/components/FormStats.vue (5 hunks)
- client/composables/forms/pendingSubmission.js (2 hunks)
- client/pages/forms/[slug]/show/stats.vue (2 hunks)
Additional comments not posted (33)
api/app/Models/Forms/FormSubmission.php (2)
14-14: LGTM!Adding
completion_timeto the$fillablearray is a good change. It allows the attribute to be mass assignable, which is useful for creating or updating records using arrays. The attribute name is clear and follows the naming convention.
21-21: Ensure type consistency when setting the attribute.Casting
completion_timeto an integer in the model is a good practice. It ensures that the attribute will always be an integer when accessing it from the model instance.Please verify that
completion_timeis being set with an integer value in other parts of the codebase where this model is being used (e.g., controllers, services, tests). This is to ensure type consistency and avoid unexpected behavior.Run the following script to verify the attribute usage:
api/database/migrations/2024_09_09_180618_add_completion_time_to_form_submissions.php (1)
1-27: LGTM!The migration file is well-structured and follows Laravel's conventions. The
completion_timecolumn is appropriately defined as an integer type and marked as nullable. Theupanddownmethods correctly handle adding and removing the column, respectively.client/components/open/forms/FormTimer.vue (1)
1-57: LGTM! The component implementation looks good.The component follows Vue 3 Composition API best practices and correctly handles the timer lifecycle based on user interactions and component lifecycle hooks. The use of
pendingSubmissioncomposable for state management is a good approach. The component exposes the necessary methods and state to be used by the parent component.client/composables/forms/pendingSubmission.js (4)
10-12: LGTM!The computed property
formPendingSubmissionTimerKeyis correctly implemented. It derives a unique key for storing the timer associated with the pending submission by suffixing the existingformPendingSubmissionKeywith "-timer".
34-37: LGTM!The
setTimerfunction is correctly implemented. It allows for setting a timer value in storage and includes a check for server-side execution to ensure that timer management only occurs in client-side contexts.
39-41: LGTM!The
removeTimerfunction is correctly implemented. It provides a convenient way to clear the timer by setting it to null using thesetTimerfunction.
43-46: LGTM!The
getTimerfunction is correctly implemented. It retrieves the current timer value from storage, providing a default value if none exists. It also includes a check for server-side execution to ensure that timer retrieval only occurs in client-side contexts.api/app/Http/Resources/FormSubmissionResource.php (1)
26-27: LGTM!The addition of the
completion_timefield enhances the data structure by providing more detailed information in the response. The logic for conditionally includingform_idandidremains unchanged, preserving the overall control flow of the method. The changes are consistent with the AI-generated summary.api/app/Http/Controllers/Forms/FormStatsController.php (3)
23-28: LGTM!The validation logic for checking the date range is correctly implemented. The error message is clear and provides actionable information to the user.
Line range hint
32-42: LGTM!The logic for retrieving form statistics for the specified date range is correctly implemented. The function correctly handles the case when the current date is included in the date range.
47-64: LGTM!The new
getFormStatsDetailsfunction is correctly implemented and provides useful statistics about the form. The function correctly handles the case when there are no submissions with a completion time recorded. The average duration is formatted appropriately for human readability using Carbon's interval formatting.api/tests/Feature/Forms/FormStatTest.php (2)
Line range hint
5-67: LGTM!The test case is correctly verifying the chart data by creating form views and submissions for past days and current day, running the database cleanup command, and checking the API response contains the expected chart data.
70-101: LGTM!The test case is correctly verifying the stats details by creating form submissions with varying completion times, including incomplete submissions, to test the average duration and completion rate calculations, creating a set number of form views to test the views count, and checking the API response contains the expected stats details.
api/app/Http/Controllers/Forms/PublicFormController.php (3)
80-81: LGTM!The formatting change improves code consistency without affecting the functionality.
92-94: LGTM!The changes improve the structure and clarity of the code by:
- Storing the validated data in a separate variable
$submissionData.- Extracting the
completion_timefield into a separate variable$completionTime.- Unsetting the
completion_timefield from the$submissionDataarray.This allows for better management of the
completion_timefield and ensures that it is treated separately when creating a new instance ofStoreFormSubmissionJob.
97-97: LGTM!The
StoreFormSubmissionJobis correctly instantiated with the modified parameters, including the new$completionTimevariable. This is consistent with the extraction ofcompletion_timeinto a separate variable and ensures that it is passed along with the form and submission data to the job.Also applies to: 101-101
client/components/open/forms/components/FormStats.vue (5)
2-15: LGTM!The code segment is well-structured and uses the
DateInputcomponent correctly. Thedisabledprop is set based on theform.is_provalue, ensuring that only professional users can filter the analytics by date range.
17-46: LGTM!The code segment is well-structured and uses conditional rendering correctly based on the
form.is_provalue. Thepro-tagandv-buttoncomponents are used appropriately for displaying the subscription prompt.
48-56: LGTM!The code segment uses conditional rendering correctly to display the loading indicator or the line chart based on the
isLoadingvalue and theform.is_provalue. TheLoaderandLineChartcomponents are used appropriately.
Line range hint
86-100: LGTM!The code segment uses the composition API correctly. The
filterFormis defined using theuseFormfunction, and thefilter_datefield is initialized appropriately. ThesubscriptionModalStoreandfilterFormare returned correctly from thesetupfunction.
Line range hint
137-181: LGTM!The code segment uses the options API correctly. The watcher on the
filterFormis defined correctly and triggers data fetching when a valid date range is selected. The initialization of the date range for professional users in themountedhook is appropriate. The update to thegetChartDatamethod to include date parameters in the API request is correct. The error handling for the API call is implemented correctly using thecatchblock and displaying an error alert using theuseAlertfunction.client/pages/forms/[slug]/show/stats.vue (1)
189-216: The script section looks good!The reactive variables and the
getCardDatafunction provide a clean and modular way to fetch and store the analytics data. CallinggetCardDatain theonMountedhook ensures that the data is fetched when the component is mounted, and the fetched data is properly assigned to the respective reactive variables.I like how you've checked if the form is valid and if the user has pro access before making the API call in the
getCardDatafunction. This is a good practice to prevent unnecessary API calls and handle access control.api/app/Jobs/Form/StoreFormSubmissionJob.php (3)
36-36: LGTM!The addition of the optional
$completionTimeparameter to the constructor is a clean and non-breaking change. The type declaration?intallows for flexibility in usage, and the default value ofnullensures backward compatibility. Good job!
73-73: Looks good!The update to the
completion_timefield of the$previousSubmissionwhen updating an existing submission is consistent with the new functionality introduced by the$completionTimeparameter. This ensures that the completion time is properly recorded for updated submissions.
79-79: Looks good!Setting the
completion_timefield when creating a new submission is consistent with the new functionality introduced by the$completionTimeparameter. This ensures that the completion time is properly recorded for new submissions.client/components/open/forms/OpenCompleteForm.vue (6)
7-10: LGTM!The addition of the
FormTimercomponent and passing theformprop to it looks good. This aligns with the PR objective of tracking form completion time.
208-208: LGTM!The import statement for the
FormTimercomponent is correct.
217-217: LGTM!The
FormTimercomponent is correctly registered in thecomponentsoption.
283-288: LGTM!The timer is correctly stopped, and the completion time is retrieved and assigned to the form data before submitting the form.
303-304: LGTM!The
completion_timeis correctly included in thesubmission_datapayload.
312-312: LGTM!The timer is correctly:
- Removed after a successful form submission.
- Reset when the form submission fails.
- Reset when the form is restarted.
This ensures that the timer is properly managed in different scenarios.
Also applies to: 332-332, 343-343
api/routes/api.php (1)
149-149: LGTM!The new route for retrieving detailed form statistics is well-defined and follows the existing conventions. It is correctly placed within the 'pro-form' middleware group to restrict access to users with a pro subscription. The route URL and name are consistent with the other form-related routes.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
api/app/Http/Controllers/Forms/FormStatsController.php (1)
53-70: Consider utilizing the date range from the request.For consistency with the
getFormStatsmethod, consider utilizing the date range from the request when calculating the statistics in this method. This would ensure that the statistics returned by both methods are based on the same date range.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- api/app/Http/Controllers/Forms/FormStatsController.php (3 hunks)
Additional comments not posted (3)
api/app/Http/Controllers/Forms/FormStatsController.php (3)
23-35: LGTM!The validation checks for the date range are correctly implemented and provide appropriate error messages to guide the user. The logic for restricting the date range to a maximum of 3 months is also correct.
Line range hint
36-50: LGTM!The logic for retrieving form statistics based on the user-defined date range is correct. The use of
CarbonPeriodto iterate over the date range is an efficient approach. The method is also correctly handling the case when the current date is included in the date range by adding the current day's views to the statistics.
53-70: LGTM!The method is correctly calculating the total views, total submissions, completion rate, and average submission duration. The use of Carbon's interval formatting to make the average duration human-readable is a nice touch. The completion rate calculation is also correctly handling the case when there are no views to avoid division by zero.
JhumanJ
force-pushed
the
6851e-better-stats
branch
from
7767733 to
80ced00
Compare
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests