Usage tracking v2#2049
Conversation
|
Warning Rate limit exceeded@truongwp has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 26 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 introduces several enhancements to the usage tracking functionality within the application. Key updates include the addition of new methods in the Changes
Possibly related PRs
Suggested reviewers
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: 7
🧹 Outside diff range and nitpick comments (6)
js/admin/usage-tracking.js (4)
1-3: Remove redundant 'use strict' directiveThe 'use strict' directive on line 2 is redundant in modern JavaScript modules, as they are automatically in strict mode. Consider removing this line to clean up the code.
Apply this diff to remove the redundant directive:
( function() { - 'use strict';🧰 Tools
🪛 Biome
[error] 2-2: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
4-10: Approved with a minor suggestion for readabilityThe event listener for tracking edit form clicked links is well-implemented. It uses event delegation efficiently and captures relevant data.
For improved readability, consider extracting the long selector into a named constant.
Here's a suggested improvement:
const EDIT_FORM_LINK_SELECTOR = '.toplevel_page_formidable #the-list .column-primary .row-actions a'; frmDom.util.documentOn('click', EDIT_FORM_LINK_SELECTOR, function(event) { // ... rest of the code });
12-18: Approved with a suggestion for error handlingThe event listener for tracking form templates is well-implemented and consistent with the previous listener. It effectively captures the template slug for tracking.
Consider adding error handling in case the closest 'li' element or its dataset.slug is not found.
Here's a suggested improvement:
frmDom.util.documentOn('click', '.frm-form-templates-use-template-button', function(event) { const listItem = event.target.closest('li'); if (listItem && listItem.dataset.slug) { sendData({ key: 'form_templates', value: listItem.dataset.slug }); } else { console.warn('Unable to find template slug for tracking'); } });
20-26: Approved with suggestions for robustnessThe
sendDatafunction is well-implemented, efficiently converting the input object to FormData and using the custom AJAX method to send the data.To improve robustness, consider adding error handling and type checking:
- Add a try-catch block to handle potential errors during the AJAX call.
- Implement type checking for the input
dataparameter.Here's a suggested improvement:
const sendData = (data) => { if (typeof data !== 'object' || data === null) { console.error('Invalid data type passed to sendData'); return; } const formData = new FormData(); Object.keys(data).forEach(key => { formData.append(key, data[key]); }); try { frmDom.ajax.doJsonPost('track_flows', formData) .catch(error => { console.error('Error sending tracking data:', error); }); } catch (error) { console.error('Error in sendData function:', error); } };This implementation adds type checking for the input data and error handling for both the AJAX call and any potential errors in the function itself.
classes/controllers/FrmHooksController.php (1)
98-99: LGTM! Consider grouping related hooks for better organization.The new action hooks for usage tracking are correctly implemented and placed appropriately within the
load_hooksmethod. They align well with the PR objectives of enhancing usage tracking functionality.For improved code organization, consider grouping related hooks together. You could move the existing usage-related hook at line 97 (
add_action( 'formidable_send_usage', 'FrmUsageController::send_snapshot' );) to be adjacent to these new hooks, creating a clear section for usage tracking functionality.classes/models/FrmUsage.php (1)
34-34: Consider reducing the request timeout durationThe timeout is set to 45 seconds, which might be longer than necessary for most HTTP requests. A shorter timeout can enhance user experience by reducing delay in case of network issues. Evaluate if a lower timeout value would be sufficient.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- classes/controllers/FrmHooksController.php (1 hunks)
- classes/controllers/FrmUsageController.php (2 hunks)
- classes/models/FrmUsage.php (5 hunks)
- formidable.php (1 hunks)
- js/admin/usage-tracking.js (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- formidable.php
🧰 Additional context used
🪛 GitHub Check: Psalm
classes/models/FrmUsage.php
[failure] 39-39: UndefinedClass
classes/models/FrmUsage.php:39:15: UndefinedClass: Class, interface or enum named FrmLog does not exist (see https://psalm.dev/019)
🪛 Biome
js/admin/usage-tracking.js
[error] 2-2: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
🔇 Additional comments (4)
js/admin/usage-tracking.js (1)
1-27: Overall assessment: Well-implemented with minor improvement suggestionsThe usage tracking implementation in this file is generally well-structured and effective. It uses event delegation for efficiency, captures relevant data, and sends it using a custom AJAX method. The suggestions provided in the review comments aim to improve:
- Code readability (extracting long selectors)
- Error handling (for both event listeners and the sendData function)
- Type checking (in the sendData function)
- Removal of redundant 'use strict' directive
Implementing these suggestions will further enhance the robustness and maintainability of the code without requiring significant changes to the overall structure or functionality.
🧰 Tools
🪛 Biome
[error] 2-2: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
classes/controllers/FrmHooksController.php (1)
Line range hint
1-379: Summary: New usage tracking hooks added successfully.The changes to
FrmHooksController.phpare minimal and focused, adding two new action hooks for usage tracking. These additions are consistent with the existing code structure and naming conventions. They enhance the plugin's ability to track usage without modifying core functionality.Key points:
- New hooks are correctly implemented and placed appropriately.
- The changes align with the PR objectives of enhancing usage tracking.
- The additions don't appear to interfere with existing functionality.
Ensure that the suggested performance and privacy checks are conducted to maintain the plugin's quality and user trust.
classes/controllers/FrmUsageController.php (1)
82-101:⚠️ Potential issueEnsure capability checks are in place for
ajax_track_flowsWhile
check_ajax_refererhelps prevent CSRF attacks, it's important to ensure that only authorized users can access this AJAX endpoint. Please verify that appropriate capability checks are implemented to prevent unauthorized access.To verify the access control for this AJAX action, you can run the following script:
classes/models/FrmUsage.php (1)
102-104: Ensure compliance with data privacy regulationsThe addition of
flows,payments, andsubscriptionsto the snapshot may involve sensitive user data. Please review and ensure that collecting and transmitting this data complies with relevant privacy laws and your company's privacy policy.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
classes/models/FrmUsage.php (1)
Line range hint
286-309: LGTM: Improved custom style handling with minor optimization opportunityThe changes to the custom style handling are good improvements:
- Creating the
FrmStyleobject outside the loop is more efficient.- Retrieving the style name instead of using the ID directly provides more meaningful data for tracking.
- The approach is more flexible and robust.
A minor optimization could be made by moving the style retrieval inside the condition:
Consider applying this diff for a small optimization:
$style = new FrmStyle(); foreach ( $saved_forms as $form ) { $new_form = array( 'form_id' => $form->id, 'description' => $form->description, 'logged_in' => $form->logged_in, 'editable' => $form->editable, 'is_template' => $form->is_template, 'entry_count' => FrmEntry::getRecordCount( $form->id ), 'field_count' => $this->form_field_count( $form->id ), 'form_action_count' => $this->form_action_count( $form->id ), ); foreach ( $settings as $setting ) { if ( isset( $form->options[ $setting ] ) ) { if ( 'custom_style' === $setting ) { $style->id = $form->options[ $setting ]; - $style_post = $style->get_one(); - $style_name = $style_post ? $style_post->post_name : 'formidable-style'; - - $new_form[ $setting ] = $style_name; + $new_form[ $setting ] = $style->get_one()->post_name ?? 'formidable-style'; } else { $new_form[ $setting ] = $this->maybe_json( $form->options[ $setting ] ); } } }This change reduces the number of variables and simplifies the code while maintaining the same functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- classes/models/FrmUsage.php (5 hunks)
🧰 Additional context used
🪛 GitHub Check: Psalm
classes/models/FrmUsage.php
[failure] 39-39: UndefinedClass
classes/models/FrmUsage.php:39:15: UndefinedClass: Class, interface or enum named FrmLog does not exist (see https://psalm.dev/019)
🔇 Additional comments (2)
classes/models/FrmUsage.php (2)
34-34: LGTM: Timeout added to prevent request failuresAdding a timeout of 45 seconds to the
wp_remote_requestoptions is a good practice. It helps prevent long-running requests from blocking the application and mitigates 'http_request_failed' errors in the Debug Log.
102-104: LGTM: Enhanced usage tracking with flows and payments dataThe addition of 'flows' and 'payments' data to the snapshot array enhances the usage tracking capabilities. The reuse of the
paymentsmethod for both payments and subscriptions demonstrates good code reuse and flexibility.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- classes/models/FrmUsage.php (5 hunks)
🧰 Additional context used
🪛 GitHub Check: Psalm
classes/models/FrmUsage.php
[failure] 39-39: UndefinedClass
classes/models/FrmUsage.php:39:15: UndefinedClass: Class, interface or enum named FrmLog does not exist (see https://psalm.dev/019)
🔇 Additional comments (3)
classes/models/FrmUsage.php (3)
34-34: Timeout addition improves request reliabilityAdding a timeout to the request options is a good practice. It helps prevent long-running requests and addresses potential
http_request_failederrors. The comment explaining the reason for this addition is helpful for future maintenance.
102-104: Enhanced data collection for usage trackingThe addition of 'flows' and payment data to the snapshot array is a good improvement. It will provide more comprehensive insights for usage tracking. The reuse of the
paymentsmethod for both regular payments and subscriptions is an efficient approach.
Line range hint
286-313: Improved handling of custom stylesThe updates to the custom style handling are well-implemented. The new code:
- Creates a new
FrmStyleobject to work with styles.- Handles both the default style (ID 1) and custom styles appropriately.
- Retrieves the style name instead of just the ID, providing more meaningful information.
This change will provide more detailed and useful information about form styles in the usage data. The code is well-structured and easy to understand.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
classes/models/FrmUsage.php (1)
102-104: LGTM: Enhanced data collection for usage trackingThe addition of flows and payments data to the snapshot is a good improvement. It will provide more comprehensive insights into the usage patterns.
Consider optimizing the
paymentsmethod calls:- 'payments' => $this->payments(), - 'subscriptions' => $this->payments( 'frm_subscriptions' ), + 'payments' => $this->payments( 'frm_payments', 'payments' ), + 'subscriptions' => $this->payments( 'frm_subscriptions', 'subscriptions' ),Then update the
paymentsmethod signature to:private function payments( $table, $key )This change would allow you to reuse the same method for both payments and subscriptions while clearly distinguishing between the two in the returned data.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- classes/models/FrmUsage.php (5 hunks)
🧰 Additional context used
🪛 GitHub Check: Psalm
classes/models/FrmUsage.php
[failure] 39-39: UndefinedClass
classes/models/FrmUsage.php:39:15: UndefinedClass: Class, interface or enum named FrmLog does not exist (see https://psalm.dev/019)
🔇 Additional comments (2)
classes/models/FrmUsage.php (2)
34-34: LGTM: Timeout added to prevent request failuresAdding a timeout to the request is a good practice. It helps prevent long-running requests and mitigates potential
http_request_failederrors. The comment explaining the purpose is helpful for future maintenance.
21-22:⚠️ Potential issueRemove commented-out testing code
The commented-out line for testing purposes should be removed from the production code. Keeping testing code in production files can lead to confusion and potential security risks if accidentally uncommented.
Apply this diff to remove the commented-out testing code:
$ep = 'aHR0cHM6Ly91c2FnZTIuZm9ybWlkYWJsZWZvcm1zLmNvbS9zbmFwc2hvdA=='; - // $ep = base64_encode( 'http://localhost:8080/snapshot' ); // Uncomment for testingLikely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
classes/controllers/FrmUsageController.php (1)
69-78: Consider adding a nonce for the tracking script.The script loading implementation looks good, but since it's handling usage tracking data, consider localizing the script with a nonce to secure the AJAX requests.
public static function load_scripts() { if ( self::is_forms_list_page() || FrmAppHelper::is_admin_page( 'formidable-form-templates' ) ) { wp_enqueue_script( 'frm-usage-tracking', FrmAppHelper::plugin_url() . '/js/admin/usage-tracking.js', array( 'formidable_dom' ), FrmAppHelper::$plug_version, true ); + wp_localize_script( + 'frm-usage-tracking', + 'frmUsageTracking', + array( + 'nonce' => wp_create_nonce( 'frm_usage_tracking' ) + ) + ); } }stubs.php (1)
382-390: Add class-level documentation.The implementation looks good with proper method documentation. Consider adding class-level PHPDoc to document the purpose of this logging class.
Add this documentation above the class:
+/** + * Logging functionality for tracking usage and issues. + * + * @since 2049 + */ class FrmLog {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- classes/controllers/FrmUsageController.php (2 hunks)
- classes/models/FrmUsage.php (5 hunks)
- stubs.php (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- classes/models/FrmUsage.php
🧰 Additional context used
🔇 Additional comments (3)
classes/controllers/FrmUsageController.php (3)
12-19: LGTM! Well-documented constant following WordPress conventions.The constant name and value are appropriately chosen for storing flows data in the options table.
80-100: LGTM! Well-structured page detection logic.The method effectively handles various edge cases and follows good practices with early returns.
128-137: LGTM! Clean and straightforward implementation.The method follows WordPress conventions and safely handles the default case.
Crabcyborg
left a comment
Crabcyborg
left a comment
There was a problem hiding this comment.
@truongwp I heard Dalton mentioning the other day in a meeting how we want to track to know if people are using AI features.
Do we have anything like that in place yet?
We'd like to know probably how many AI fields are being used? And how often they're building an AI-generated form.
Can we track those things as well?
Thank you!
|
@Crabcyborg This PR doesn't track the AI-generated form. It will be in the AI addon. There are 2 ways to track it:
Which way do you prefer? I won't create a new database table for this. I will merge AI-generated form data into the form templates, and AI-generated forms will have the |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- classes/models/FrmUsage.php (5 hunks)
🔇 Additional comments (2)
classes/models/FrmUsage.php (2)
34-34: LGTM: Good addition of timeout parameter.
The addition of a 45-second timeout parameter is a good improvement to prevent request failures and Debug Log errors.
117-132: LGTM: Good implementation of data filtering.
The method effectively filters out sensitive and unnecessary data keys, maintaining anonymity in usage tracking. This aligns well with the privacy-focused approach mentioned in the PR comments.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
js/admin/usage-tracking.js (2)
1-3: Remove redundant 'use strict' directiveThe 'use strict' directive is unnecessary as JavaScript modules are automatically in strict mode.
( function() { - 'use strict'; -🧰 Tools
🪛 Biome
[error] 2-2: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
4-7: Consider using data attributes for action trackingWhile the current implementation works, relying on className for tracking actions can be fragile if CSS classes change. Consider using dedicated data attributes for tracking purposes.
- sendData( 'edit_form_clicked_link', event.target.parentNode.className ); + sendData( 'edit_form_clicked_link', event.target.parentNode.dataset.action || event.target.parentNode.className );Also add the data attribute to your action links:
<a href="#" data-action="edit">Edit</a>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
js/admin/usage-tracking.js(1 hunks)
🧰 Additional context used
🪛 Biome
js/admin/usage-tracking.js
[error] 2-2: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
🔇 Additional comments (2)
js/admin/usage-tracking.js (2)
9-12: LGTM! Well-structured template tracking implementation.
Good use of data attributes for template identification and event delegation for performance.
14-19: LGTM! Well-structured data transmission.
The implementation using separate 'key' and 'value' fields in FormData provides clear structure for server-side processing. This approach makes it easier to track and process different types of events consistently.
Let's verify the server-side handling of these tracking events:
✅ Verification successful
Server-side implementation properly handles the tracking data
The server-side implementation in FrmUsageController properly validates and processes the tracking data:
- Performs permission checks and nonce verification for security
- Sanitizes both key and value parameters
- Validates non-empty values before storage
- Maintains structured data organization through the
update_flows_datamethod
The client-side implementation using separate 'key' and 'value' fields aligns well with the server-side handling, making this a robust tracking solution.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the AJAX handler that processes these tracking events
ast-grep --pattern 'function ajax_track_flows($$$)'
Length of output: 53
Script:
#!/bin/bash
# Search for track_flows in PHP files using ripgrep
rg -t php "track_flows" -A 5
# Also search for any AJAX action registration related to track_flows
rg -t php "add_action.*track_flows"
Length of output: 1126
Script:
#!/bin/bash
# Let's see the complete implementation of the ajax_track_flows method
rg -t php -B 2 -A 10 "ajax_track_flows"
Length of output: 2000
Script:
#!/bin/bash
# Let's check the update_flows_data method implementation
rg -t php -B 2 -A 10 "update_flows_data"
Length of output: 1662
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
classes/models/FrmUsage.php (3)
21-22: Remove or secure the commented testing endpointThe commented-out testing endpoint should be removed from production code to prevent accidental exposure of internal endpoints.
$ep = 'aHR0cHM6Ly91c2FnZTIuZm9ybWlkYWJsZWZvcm1zLmNvbS9zbmFwc2hvdA=='; - // $ep = base64_encode( 'http://localhost:4567/snapshot' ); // Uncomment for testing
122-137: Optimize data filtering using array_diff_keyThe current implementation using foreach to unset keys can be simplified.
private function onboarding_wizard() { $data = FrmOnboardingWizardController::get_usage_data(); $skipped_keys = array( 'default_email', 'is_subscribed', 'allows_tracking', 'summary_emails', 'installed_addons', ); - - foreach ( $skipped_keys as $skipped_key ) { - unset( $data[ $skipped_key ] ); - } - - return $data; + return array_diff_key( $data, array_flip( $skipped_keys ) ); }
344-357: Simplify style name determination logicConsider extracting the style name logic to a dedicated method in the
FrmStyleclass.if ( 'custom_style' === $setting ) { $style->id = $form->options[ $setting ]; - - if ( ! $style->id ) { - $style_name = 0; - } elseif ( 1 === intval( $style->id ) ) { - $style_name = 'formidable-style'; - } else { - $style_post = $style->get_one(); - $style_name = $style_post ? $style_post->post_name : 'formidable-style'; - } - - $new_form[ $setting ] = $style_name; + $new_form[ $setting ] = $style->get_style_name();Add this method to the
FrmStyleclass:public function get_style_name() { if ( ! $this->id ) { return 0; } if ( 1 === intval( $this->id ) ) { return 'formidable-style'; } $style_post = $this->get_one(); return $style_post ? $style_post->post_name : 'formidable-style'; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
classes/models/FrmUsage.php(5 hunks)
🔇 Additional comments (4)
classes/models/FrmUsage.php (4)
37-41: LGTM: Time limit removal for large datasets
Good addition to prevent timeout issues when processing large amounts of data.
42-51: Verify FrmLog class availability
The error logging implementation looks good, but ensure the FrmLog class is properly defined to prevent runtime errors.
✅ Verification successful
Let me check the implementation of FrmLog class to ensure it has the required methods.
FrmLog class implementation is properly defined
The FrmLog class is defined in stubs.php with the required add() method that accepts an array parameter, which matches the usage in FrmUsage.php. The implementation is correct and safe to use.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for the FrmLog class definition
rg --type php 'class FrmLog'
# If not found directly, check for any FrmLog references or potential includes
rg --type php 'FrmLog'
Length of output: 439
Script:
#!/bin/bash
# Check the content of stubs.php for FrmLog implementation
cat stubs.php
Length of output: 10411
147-166:
Secure database query using $wpdb->prepare
While the table whitelist is good, it's safer to use $wpdb->prepare() for query construction.
private function payments( $table = 'frm_payments' ) {
$allowed_tables = array( 'frm_payments', 'frm_subscriptions' );
if ( ! in_array( $table, $allowed_tables, true ) ) {
return array();
}
global $wpdb;
- $rows = $wpdb->get_results( "SELECT amount, status, paysys, created_at FROM {$wpdb->prefix}{$table}" );
+ $query = $wpdb->prepare( "SELECT amount, status, paysys, created_at FROM {$wpdb->prefix}%s", $table );
+ $rows = $wpdb->get_results( $query );
$payments = array();Likely invalid or redundant comment.
344-357: Verify handling of disabled styling
The style name determination logic should account for cases where styling is disabled.
✅ Verification successful
Let me gather more information about the style handling logic to understand how disabled styling is managed.
Based on the search results, I can now provide a final response about the handling of disabled styling:
Style handling is properly implemented for disabled cases
The code correctly handles disabled styling through multiple checks:
- When
$style->idis falsy (0 or empty), it sets$style_name = 0which indicates disabled styling - This aligns with the global settings where
load_style = 'none'disables styles globally - The style handling is also validated through
FrmFormsHelper::get_form_style()which properly processes both per-form and global style settings
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for style disable functionality
rg --type php 'disable.*style|style.*disable'
Length of output: 3154
Script:
#!/bin/bash
# Check style handling in FrmUsage class and related files
rg --type php -A 5 'custom_style'
Length of output: 29901
|
@truongwp Before I merge this, can you help with the PHPCS errors? 
Thank you! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
classes/models/FrmUsage.php (1)
21-21: LGTM! Consider monitoring the timeout value.The implementation includes good practices:
- Error logging with proper class existence check
- Base64 encoded endpoint URL
- Removal of execution time limit for long-running requests
The 45-second timeout should be sufficient but consider monitoring if requests consistently take this long.
Also applies to: 34-52
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
classes/models/FrmUsage.php(5 hunks)
🔇 Additional comments (2)
classes/models/FrmUsage.php (2)
116-138: LGTM! Good data privacy practices.
The method effectively filters out sensitive and unnecessary data before sending it for tracking.
351-364: LGTM! Comprehensive style handling.
The implementation correctly handles all style cases:
- Disabled styling (id = 0)
- Default style (id = 1)
- Custom styles
- Fallback to default style name
|
@Crabcyborg I fixed those errors. This PR logs the failed usage requests. Do you think we should keep it? |
@truongwp I'm not sure. If there are issues, it would help make them more visible. But it also isn't something that our customers shouldn't need to worry about. I think we're fine to just remove it. |
I removed the code to log the failed requests. Thank you. |
2 participants
This PR:
https://usage2.formidableforms.com/snapshot.