New sales API#2205
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## lite-to-pro-improvement #2205 +/- ##
==========================================================
Coverage ? 26.82%
Complexity ? 8288
==========================================================
Files ? 129
Lines ? 27415
Branches ? 0
==========================================================
Hits ? 7353
Misses ? 20062
Partials ? 0 ☔ View full report in Codecov by Sentry. |
WalkthroughThe pull request introduces a comprehensive set of changes across multiple files in the Formidable Forms plugin, focusing on enhancing the sales and upgrade mechanisms. A new Changes
Sequence DiagramsequenceDiagram
participant User
participant SalesApi
participant AppHelper
participant ViewTemplate
User->>SalesApi: Request best sale
SalesApi-->>SalesApi: Check cached sales data
SalesApi->>SalesApi: Retrieve/Generate sales data
SalesApi-->>ViewTemplate: Return best sale details
ViewTemplate->>AppHelper: Fallback link generation
ViewTemplate->>User: Display dynamic upgrade options
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
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: 4
🧹 Nitpick comments (7)
classes/views/dashboard/templates/pro-features-list.php (1)
21-29: Consider adding error logging for failed API calls.While the fallback mechanism is good, it would be helpful to log when the API call fails to help with debugging.
$discount_link = FrmSalesApi::get_best_sale_value( 'global_settings_upgrade_cta_link' ); if ( ! $discount_link ) { + error_log( 'FrmSalesApi::get_best_sale_value failed to retrieve global_settings_upgrade_cta_link' ); $discount_link = FrmAppHelper::admin_upgrade_link(classes/views/frm-settings/license_box.php (1)
6-19: Consider consolidating repeated code patterns.The code repeats the same pattern four times (get value from API, check if empty, use fallback). Consider extracting this pattern into a helper method.
+function get_sales_value_with_fallback($key, $fallback_callback) { + $value = FrmSalesApi::get_best_sale_value($key); + return $value ?: $fallback_callback(); +} -$button_upgrade_text = FrmSalesApi::get_best_sale_value( 'global_settings_license_cta_text' ); -if ( ! $button_upgrade_text ) { - $button_upgrade_text = __( 'Get Formidable Now', 'formidable' ); -} +$button_upgrade_text = get_sales_value_with_fallback( + 'global_settings_license_cta_text', + function() { return __( 'Get Formidable Now', 'formidable' ); } +);Also applies to: 21-34
classes/models/FrmSalesAPI.php (5)
9-30: Add complete PHPDoc for class properties.Consider adding comprehensive PHPDoc blocks for all class properties to improve code maintainability and IDE support.
/** + * Class for managing sales data from API. + * * @since x.x + * @property-read string $cache_key Cache key for storing sales data + * @property-read array|false $sales All sales from API data + * @property-read array|false|null $best_sale Best sale from API data */ class FrmSalesApi extends FrmFormApi {
32-48: Consider making cache key and API URL configurable.The hardcoded values for cache key and API URL could be moved to configuration constants or settings to improve flexibility and maintainability.
+ const CACHE_KEY = 'frm_sales_cache'; + const API_URL = 'https://formidableforms.com/wp-json/s11-sales/v1/list/'; protected function set_cache_key() { - $this->cache_key = 'frm_sales_cache'; + $this->cache_key = defined('FRM_SALES_CACHE_KEY') ? FRM_SALES_CACHE_KEY : self::CACHE_KEY; } protected function api_url() { - return 'https://formidableforms.com/wp-json/s11-sales/v1/list/'; + return defined('FRM_SALES_API_URL') ? FRM_SALES_API_URL : self::API_URL; }Also applies to: 54-56
63-124: Enhance data validation in sales management.The sales data validation could be strengthened to ensure data integrity:
- Validate numeric fields (discount_percent)
- Validate URL fields
- Validate timestamp fields (starts, expires)
private function fill_sale( $sale ) { $defaults = array( 'key' => '', 'starts' => '', 'expires' => '', 'who' => 'all', - 'discount_percent' => 0, + 'discount_percent' => 0.0, 'test_group' => '', 'lite_banner_cta_link' => '', // ... other fields ... ); + // Validate numeric fields + $sale['discount_percent'] = filter_var($sale['discount_percent'], FILTER_VALIDATE_FLOAT); + + // Validate timestamps + foreach (['starts', 'expires'] as $field) { + if (!empty($sale[$field]) && !is_numeric($sale[$field])) { + $sale[$field] = strtotime($sale[$field]); + } + } + + // Validate URLs + foreach (array_keys($defaults) as $field) { + if (strpos($field, '_link') !== false && !empty($sale[$field])) { + $sale[$field] = filter_var($sale[$field], FILTER_VALIDATE_URL); + } + } return array_merge( $defaults, $sale ); }🧰 Tools
🪛 GitHub Check: Psalm
[failure] 92-92: PossiblyInvalidArrayAssignment
classes/models/FrmSalesAPI.php:92:3: PossiblyInvalidArrayAssignment: Cannot access array value on non-array variable FrmSalesApi::$sales[mixed] of type false (see https://psalm.dev/118)🪛 GitHub Actions: Psalm Code Analysis
[error] 92-92: PossiblyInvalidArrayAssignment: Cannot access array value on non-array variable FrmSalesApi::$sales[mixed] of type false
147-173: Enhance best sale selection logic.The get_best_sale() method could be improved to handle edge cases and provide more flexibility in sale selection:
- Consider sale priority in addition to discount percentage
- Add support for minimum purchase requirements
- Cache the result for better performance
public function get_best_sale() { + $cache_key = 'frm_best_sale_' . md5(serialize(array($this->get_ab_group_for_current_site()))); + $cached = wp_cache_get($cache_key); + if ($cached !== false) { + return $cached; + } if ( ! self::$sales ) { return false; } if ( isset( self::$best_sale ) ) { return self::$best_sale; } $best_sale = false; foreach ( self::$sales as $sale ) { if ( ! FrmApiHelper::is_for_user( $sale ) ) { continue; } if ( ! $this->matches_ab_group( $sale ) ) { continue; } + // Consider sale priority + $priority = isset($sale['priority']) ? $sale['priority'] : 0; + + // Check minimum purchase if set + $min_purchase = isset($sale['min_purchase']) ? $sale['min_purchase'] : 0; + if ($min_purchase > 0 && $this->get_cart_total() < $min_purchase) { + continue; + } - if ( ! $best_sale || $sale['discount_percent'] > $best_sale['discount_percent'] ) { + if ( ! $best_sale || + $priority > $best_sale['priority'] || + ($priority == $best_sale['priority'] && $sale['discount_percent'] > $best_sale['discount_percent']) + ) { $best_sale = $sale; } } self::$best_sale = $best_sale; + wp_cache_set($cache_key, self::$best_sale, '', 300); // Cache for 5 minutes return self::$best_sale; }
199-222: Enhance A/B testing capabilities.The A/B testing implementation could be improved with:
- Support for multiple test groups beyond binary 0/1
- Persistent user group assignment
- Traffic distribution control
private function matches_ab_group( $sale ) { if ( ! is_numeric( $sale['test_group'] ) ) { return true; } + $test_groups = $this->get_test_groups(); + if (!in_array($sale['test_group'], array_keys($test_groups))) { + return false; + } $ab_group = $this->get_ab_group_for_current_site(); return $ab_group === $sale['test_group']; } + private function get_test_groups() { + return apply_filters('frm_sale_test_groups', array( + 0 => array('name' => 'Control', 'weight' => 50), + 1 => array('name' => 'Test A', 'weight' => 50), + )); + } private function get_ab_group_for_current_site() { $option = get_option( 'frm_sale_ab_group' ); if ( ! is_numeric( $option ) ) { - $option = mt_rand( 0, 1 ); + $groups = $this->get_test_groups(); + $weights = wp_list_pluck($groups, 'weight'); + $option = $this->weighted_random($weights); update_option( 'frm_sale_ab_group', $option, false ); } return (int) $option; } + private function weighted_random($weights) { + $r = mt_rand(1, array_sum($weights)); + $offset = 0; + foreach ($weights as $k => $w) { + $offset += $w; + if ($r <= $offset) { + return $k; + } + } + return 0; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
classes/controllers/FrmAddonsController.php(2 hunks)classes/controllers/FrmAppController.php(1 hunks)classes/helpers/FrmApiHelper.php(1 hunks)classes/helpers/FrmAppHelper.php(1 hunks)classes/helpers/FrmDashboardHelper.php(2 hunks)classes/models/FrmAddon.php(1 hunks)classes/models/FrmInbox.php(2 hunks)classes/models/FrmSalesAPI.php(1 hunks)classes/views/dashboard/templates/pro-features-list.php(2 hunks)classes/views/frm-settings/license_box.php(3 hunks)classes/views/shared/admin-footer-links.php(2 hunks)phpstan.neon(0 hunks)
💤 Files with no reviewable changes (1)
- phpstan.neon
👮 Files not reviewed due to content moderation or server errors (2)
- classes/controllers/FrmAddonsController.php
- classes/helpers/FrmAppHelper.php
🧰 Additional context used
🪛 GitHub Check: Psalm
classes/models/FrmSalesAPI.php
[failure] 92-92: PossiblyInvalidArrayAssignment
classes/models/FrmSalesAPI.php:92:3: PossiblyInvalidArrayAssignment: Cannot access array value on non-array variable FrmSalesApi::$sales[mixed] of type false (see https://psalm.dev/118)
🪛 GitHub Actions: Psalm Code Analysis
classes/models/FrmSalesAPI.php
[error] 92-92: PossiblyInvalidArrayAssignment: Cannot access array value on non-array variable FrmSalesApi::$sales[mixed] of type false
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Cypress
- GitHub Check: PHP 8 tests in WP trunk
🔇 Additional comments (6)
classes/views/dashboard/templates/pro-features-list.php (1)
13-19: LGTM! Well-implemented dynamic discount calculation.The implementation correctly handles the dynamic discount calculation with a proper fallback to 50% when no better discount is available. The translation string is properly formatted with a placeholder.
classes/views/shared/admin-footer-links.php (1)
9-13: LGTM! Proper security measures in place.The implementation correctly uses escaping functions (esc_url, esc_html) and follows WordPress security best practices.
Also applies to: 32-39
classes/helpers/FrmApiHelper.php (1)
89-98: Consider timezone handling in timestamp comparison.Given that PR objective #3 mentions timezone changes from UTC to New York, the timestamp comparison should account for timezone differences.
✅ Verification successful
Timezone handling is correctly implemented
The current implementation is safe because:
- It uses relative time calculation (-30 days) which is timezone-independent
- WordPress stores options in UTC by default, ensuring consistent timestamp comparisons
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if timezone handling is consistent across the codebase rg -l "strtotime|time\(|date\(" | xargs rg "new york|america|timezone|UTC"Length of output: 76
Script:
#!/bin/bash # Check for WordPress timezone functions and settings rg "get_option\(['\"]timezone|get_option\(['\"]gmt_offset|wp_timezone|current_time" # Look for similar timestamp comparisons ast-grep --pattern 'strtotime("-$_ days")' # Check for any date/time configuration in wp-config or main plugin file fd -e php -e ini -e json "config|settings" --exec rg "timezone|UTC|date\.timezone"Length of output: 1624
classes/models/FrmInbox.php (1)
208-211: LGTM! Good refactoring of user eligibility checks.The delegation of user eligibility checks to FrmApiHelper improves code organization and reduces duplication.
classes/helpers/FrmDashboardHelper.php (1)
199-212: LGTM! Good implementation of dynamic sales content.The changes properly integrate with FrmSalesApi while maintaining fallback options for CTA text and upgrade links.
Also applies to: 221-221
classes/models/FrmAddon.php (1)
333-335: LGTM! Proper cache management.The addition of FrmSalesApi cache reset ensures consistent cache management across the application.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
classes/models/FrmSalesAPI.php (2)
129-130: Ensure 'starts' and 'expires' are integers after merging.After merging defaults with API data in
fill_sale,'starts'and'expires'may not be integers. Casting these values ensures proper comparison insale_is_active.Apply this diff to cast
'starts'and'expires'to integers:private function fill_sale( $sale ) { $defaults = array( // ... existing defaults ); $filled_sale = array_merge( $defaults, $sale ); + $filled_sale['starts'] = (int) $filled_sale['starts']; + $filled_sale['expires'] = (int) $filled_sale['expires']; return $filled_sale; }
6-7: Update@sinceannotations with the correct version number.The
@since x.xannotations are placeholders. Please replace'x.x'with the appropriate version numbers before release to maintain accurate documentation.Also applies to: 41-44, 49-56, 135-139, 184-188
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
classes/models/FrmSalesAPI.php(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: Cypress
🔇 Additional comments (1)
classes/models/FrmSalesAPI.php (1)
140-146: Improve timezone handling.The hardcoded
'America/New_York'timezone should be configurable and consider the site's timezone settings.Apply this diff to make the timezone configurable:
private function sale_is_active( $sale ) { $starts = $sale['starts']; $expires = $sale['expires']; - $date = new DateTime( 'now', new DateTimeZone( 'America/New_York' ) ); + $timezone = apply_filters( 'frm_sales_timezone', get_option( 'timezone_string' ) ?: 'UTC' ); + $date = new DateTime( 'now', new DateTimeZone( $timezone ) ); $today = $date->getTimestamp(); return $today >= $starts && $today <= $expires; }
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
classes/models/FrmSalesAPI.php (3)
7-8: Add version number in PHPDoc.The
@since x.xtag should be replaced with the actual version number where this class was introduced.- * @since x.x + * @since 6.0.0
54-56: Consider making the API URL configurable.The API URL is hardcoded. Consider making it configurable through constants or environment variables for better flexibility across different environments.
protected function api_url() { - return 'https://plapi.formidableforms.com/sales/'; + return defined('FRM_SALES_API_URL') ? FRM_SALES_API_URL : 'https://plapi.formidableforms.com/sales/'; }
205-213: Add validation for test group values.The
matches_ab_groupmethod should validate that the test group is either 0 or 1.private function matches_ab_group( $sale ) { - if ( ! is_numeric( $sale['test_group'] ) ) { + if ( ! is_numeric( $sale['test_group'] ) || + ! in_array( (int) $sale['test_group'], [0, 1], true ) ) { // No test group, so return true. return true; } $ab_group = $this->get_ab_group_for_current_site(); return $ab_group === (int) $sale['test_group']; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
classes/models/FrmSalesAPI.php(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: Cypress
🔇 Additional comments (2)
classes/models/FrmSalesAPI.php (2)
107-109: Set default values for 'starts' and 'expires'.The default values for
'starts'and'expires'should be meaningful timestamps.
140-146: 🛠️ Refactor suggestionValidate timestamps and add timezone constant.
The timezone is hardcoded and timestamps are used without validation.
private function sale_is_active( $sale ) { + if ( empty( $sale['starts'] ) || empty( $sale['expires'] ) || + !is_numeric( $sale['starts'] ) || !is_numeric( $sale['expires'] ) ) { + return false; + } + $timezone = defined('FRM_TIMEZONE') ? FRM_TIMEZONE : 'America/New_York'; $starts = $sale['starts']; $expires = $sale['expires']; - $date = new DateTime( 'now', new DateTimeZone( 'America/New_York' ) ); + $date = new DateTime( 'now', new DateTimeZone( $timezone ) ); $today = $date->getTimestamp(); return $today >= $starts && $today <= $expires; }Likely invalid or redundant comment.
1 participant
Fixes https://github.com/Strategy11/formidable-pro/issues/5568
TODO
Completed