Add support for limit related addons #180
Conversation
# Conflicts: # readme.txt
WalkthroughAdds vendor library updates (ApexCharts, Shepherd), new interfaces (Billable, Notable), updates models to implement interfaces, moves Limitable to an Interfaces namespace, refines Sunrise meta loading, improves Limitations initialization, adds a dashboard hook, updates build scripts/translations, and introduces extensive PHPUnit tests and a README badge. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Dev/Caller
participant Apex as ApexCharts
participant Parser as Data Parser
participant Theme as Theme Palettes
participant Export as Export/Styles
Dev->>Apex: init/update(options, series)
Apex->>Parser: parseRawDataIfNeeded(series)
Parser-->>Apex: normalized data + parsed flags
Apex->>Theme: constructor.getThemePalettes()
Theme-->>Apex: palettes
alt non-axis charts (pie/donut/radialBar)
Apex->>Parser: extractPieDataFromSeries(...)
Parser-->>Apex: values + labels
end
Dev->>Export: exportSVG/PNG request
Export->>Apex: query legend visibility
Apex-->>Export: legend state
Export->>Export: inject CSS only when allowed
sequenceDiagram
autonumber
actor User
participant Shepherd as Shepherd Element
participant Attach as attachToElement
participant Dialog as Dialog
User->>Shepherd: open step / reposition
Shepherd->>Attach: compute/bind focusables, keydown
Shepherd->>Dialog: compute focusables, set tabIndex before focus
Note over Shepherd,Dialog: tab navigation fenced between attachTo focusables and dialog focusables
User->>Shepherd: Tab / Shift+Tab
Shepherd->>Shepherd: move or wrap focus accordingly
Shepherd->>Shepherd: onDestroy cleanup bindings
sequenceDiagram
autonumber
participant Sunrise as Sunrise
participant WP as NetworkOptions
Sunrise->>Sunrise: load_dependencies()
Sunrise->>WP: get_network_option('wu_sunrise_meta', defaults)
WP-->>Sunrise: meta (ensured keys)
Sunrise-->>Sunrise: set self::$sunrise_meta
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
assets/js/lib/shepherd.js (1)
4527-4535: Ensure Svelte component destroyed in Step.destroy to prevent memory leaksI confirmed via a repository-wide grep that no existing calls to
shepherdElementComponent.$destroyor alternate teardown sites exist, so we need to add one.• File: assets/js/lib/shepherd.js
• Method: Step.destroy() (around lines 4527–4535)Apply this diff to invoke the Svelte component’s
$destroy()and clear its reference:destroy() { destroyTooltip(this); if (isHTMLElement$1(this.el)) { this.el.remove(); this.el = null; } + // Ensure the Svelte component instance is torn down and its onDestroy hooks fire + if (this.shepherdElementComponent && typeof this.shepherdElementComponent.$destroy === 'function') { + this.shepherdElementComponent.$destroy(); + this.shepherdElementComponent = null; + } this._updateStepTargetOnHide(); this.trigger('destroy'); }This guarantees that any event listeners or cleanup logic in the Svelte component’s
onDestroyrun, avoiding stale listeners and memory buildup when steps are shown and hidden repeatedly.inc/managers/class-notes-manager.php (1)
371-374: Return a structured error when the target object is missingSilently
return;makes the AJAX UX ambiguous. Respond with a proper JSON error so callers can react accordingly.- if ( ! $object) { - return; - } + if ( ! $object) { + wp_send_json_error(new \WP_Error('not-found', __('Object not found', 'multisite-ultimate'))); + }inc/objects/class-limitations.php (1)
109-113: Use canonical magic method signature for __serializeDeclare the return type to match PHP’s magic method signature. This also helps static analyzers.
- public function __serialize() { + public function __serialize(): array {inc/class-sunrise.php (1)
351-370: Cast gmdate() inputs to int in system_info (prevent PHP 8 TypeError)Under PHP 8, gmdate’s second parameter must be an int or null—passing a string (e.g. the default
'unknown'or a legacy/non-numeric value) will trigger a fatal TypeError (php.net, lists.wordpress.org). InSunrise::system_info(), the$data[...]entries forcreated,last_activated,last_deactivated, andlast_modifiedcan be non-numeric (legacy meta or user-edited), so they must be cast to int before formatting.Please apply the following changes in inc/class-sunrise.php at the gmdate calls in
system_info():• Lines ~353–370:
- 'value' => gmdate('Y-m-d @ H:i:s', $data['created']), + 'value' => gmdate('Y-m-d @ H:i:s', (int) $data['created']), - 'value' => gmdate('Y-m-d @ H:i:s', $data['last_activated']), + 'value' => gmdate('Y-m-d @ H:i:s', (int) $data['last_activated']), - 'value' => gmdate('Y-m-d @ H:i:s', $data['last_deactivated']), + 'value' => gmdate('Y-m-d @ H:i:s', (int) $data['last_deactivated']), - 'value' => gmdate('Y-m-d @ H:i:s', $data['last_modified']), + 'value' => gmdate('Y-m-d @ H:i:s', (int) $data['last_modified']),• Verify that all four gmdate calls in
system_info()now wrap their$dataindices in(int).After merging, please confirm that the full test suite and deployment pipeline pass without errors, ensuring resilience against malformed or legacy meta values.
inc/managers/class-limitation-manager.php (1)
497-505: Fatal risk: passing array element by reference via arrow function + by-ref parameter.
get_theme_selection_list($obj, &$section)expects a reference, but the call passes$sections['allowed_themes']from inside an arrow function. Two problems:
- Arrow functions capture
$sectionsby value at creation time, soallowed_themesis undefined when executed.- PHP does not allow passing an array-element expression by reference (“Only variables can be passed by reference”).
Switch to a normal closure capturing
$sectionsby reference and bind the element to a variable before passing.Apply this diff:
- 'content' => fn() => $this->get_theme_selection_list($object_model, $sections['allowed_themes']), + 'content' => function () use ($object_model, &$sections) { + $section = &$sections['allowed_themes']; + return $this->get_theme_selection_list($object_model, $section); + },
🧹 Nitpick comments (48)
README.md (2)
21-21: Correct the alt text for accessibility and accuracy.The image’s alt currently says “E2E Tests” but this badge represents “CodeRabbit Reviews.”
Apply this diff to fix the label:
- <a href="https://img.shields.io/coderabbit/prs/github/superdav42/wp-multisite-waas?utm_source=oss&utm_medium=github&utm_campaign=superdav42%2Fwp-multisite-waas&labelColor=171717&color=FF570A&link=https%3A%2F%2Fcoderabbit.ai&label=CodeRabbit+Reviews"><img src="https://img.shields.io/coderabbit/prs/github/superdav42/wp-multisite-waas?utm_source=oss&utm_medium=github&utm_campaign=superdav42%2Fwp-multisite-waas&labelColor=171717&color=FF570A&link=https%3A%2F%2Fcoderabbit.ai&label=CodeRabbit+Reviews" alt="E2E Tests"></a> + <a href="https://img.shields.io/coderabbit/prs/github/superdav42/wp-multisite-waas?utm_source=oss&utm_medium=github&utm_campaign=superdav42%2Fwp-multisite-waas&labelColor=171717&color=FF570A&link=https%3A%2F%2Fcoderabbit.ai&label=CodeRabbit+Reviews"><img src="https://img.shields.io/coderabbit/prs/github/superdav42/wp-multisite-waas?utm_source=oss&utm_medium=github&utm_campaign=superdav42%2Fwp-multisite-waas&labelColor=171717&color=FF570A&link=https%3A%2F%2Fcoderabbit.ai&label=CodeRabbit+Reviews" alt="CodeRabbit Reviews"></a>
21-21: Make the badge click-through go to CodeRabbit (not the Shields endpoint).Right now the anchor href points to the Shields image URL. For a better UX and to match the other badges (which link to their relevant pages), point the anchor to CodeRabbit. The embedded SVG already has a link param; keep it or simplify, but the outer anchor should target CodeRabbit.
Apply this diff to update the href and simplify the img query a bit:
- <a href="https://img.shields.io/coderabbit/prs/github/superdav42/wp-multisite-waas?utm_source=oss&utm_medium=github&utm_campaign=superdav42%2Fwp-multisite-waas&labelColor=171717&color=FF570A&link=https%3A%2F%2Fcoderabbit.ai&label=CodeRabbit+Reviews"><img src="https://img.shields.io/coderabbit/prs/github/superdav42/wp-multisite-waas?utm_source=oss&utm_medium=github&utm_campaign=superdav42%2Fwp-multisite-waas&labelColor=171717&color=FF570A&link=https%3A%2F%2Fcoderabbit.ai&label=CodeRabbit+Reviews" alt="CodeRabbit Reviews"></a> + <a href="https://coderabbit.ai"><img src="https://img.shields.io/coderabbit/prs/github/superdav42/wp-multisite-waas?utm_source=oss&utm_medium=github&utm_campaign=superdav42%2Fwp-multisite-waas&labelColor=171717&color=FF570A&label=CodeRabbit+Reviews" alt="CodeRabbit Reviews"></a>If you prefer deep-linking to the repo’s reviews page, replace the href with that specific URL.
assets/js/lib/shepherd.js (3)
2390-2390: Focus safety: avoid clobbering an existing tabindexSetting step.el.tabIndex = 0 ensures focusability before focusing, but it overwrites any pre-existing tabindex defined by themes/integrators. Prefer only setting when absent, and consider restoring the original value on teardown.
Apply this diff to minimize impact:
- if (step != null && step.el) { - step.el.tabIndex = 0; + if (step != null && step.el) { + if (step.el.getAttribute('tabindex') === null) { + step.el.tabIndex = 0; + }Additionally, store any original tabindex on creation and restore it when destroying the step.
4359-4388: Keyboard trap across attachTo + dialog: good coverage, one nitThe wrap-around logic between first/last focusable in attachTo and dialog looks sound. Nit: keyCode is deprecated; consider using e.key ('Tab', 'Escape', etc.) for modern browsers while keeping a fallback for legacy.
4278-4292: Public surface expansion on Shepherd_element props — trim unused propattachTofocusableDialogElements is exposed but unused. Consider removing it from the public API to avoid confusion.
assets/js/lib/apexcharts.js (4)
10350-10364: Axis grouping can become unstable between parsed/unparsed seriesSwitching the default group key to
apexcharts-axis-<seriesIndex>when__apexParsedis set changes grouping semantics vs. the axis-index-based keyapexcharts-axis-<axisIndex>. This can lead to series bouncing between groups across parse/reset cycles and multi-axis configs.
- Keep grouping based on axis index for stability.
- If uniqueness is required, attach a suffix but preserve the axis root (e.g.,
apexcharts-axis-<ai>-s<si>), and ensure the rest of the code doesn't assume strict equality on the whole string.Would you like me to draft a targeted patch after confirming expectations for group keys?
16182-16183: Legend CSS bundling works—consider optional externalizationInlining
css_248z$1is fine for portability. If bundle size becomes a concern, consider external stylesheet for UI-only styles. Not blocking.
36619-36636: Theme palettes method added—coordinate with the callerMethod is added as an instance method. With the fix in the Theme class (above), this will work. If a static API was intended across the codebase, declare and use it consistently.
- }, { - key: "getThemePalettes", - value: function getThemePalettes() { + }, { + key: "getThemePalettes", + value: function getThemePalettes() { return { palette1: ['#008FFB', '#00E396', '#FEB019', '#FF4560', '#775DD0'], ... }; }
1-6: Avoid direct edits to vendored bundles; keep a maintainable patch strategyPatching
assets/js/lib/apexcharts.jsdirectly increases upgrade friction and risk of drift. Prefer:
- Keeping local patches as small, clearly-delimited diffs that you can reapply on vendor updates, or
- Extending via public ApexCharts APIs, or
- Wrapping/monkey-patching at initialization time in a separate module (e.g.,
apexcharts-patches.js) and importing it where needed.I can help extract these changes into an isolated patch file and wire it into the build.
inc/models/interfaces/interface-limitable.php (1)
17-21: Docblocks still refer to “trait”; update to “interface” for accuracyMultiple comments say “imports this trait” and “models that have the trait Limitable,” but this file defines an interface. Update wording to avoid confusion with the similarly named trait.
Apply diffs:
- * Every model that is limitable (imports this trait) + * Every model that is limitable (implements this interface) - * for all models that have the trait Limitable. + * for all models that implement the Limitable interface (often using the Limitable trait).Also applies to: 84-86
inc/models/class-product.php (1)
16-16: Import alias to reduce ambiguity between interface and trait named “Limitable”This file imports the interface Limitable and also uses the trait Traits\Limitable. To avoid name shadowing and improve readability, alias the interface and use that alias in the implements clause.
Apply this change to the import:
-use WP_Ultimo\Models\Interfaces\Limitable; +use WP_Ultimo\Models\Interfaces\Limitable as LimitableInterface;And update the class signature (outside this hunk) accordingly:
class Product extends Base_Model implements LimitableInterface { use Traits\Limitable; // ... }inc/admin-pages/class-edit-admin-page.php (1)
18-18: Importing the interface is correct; consider aliasing for clarity and update instanceofConsistent with the namespace move. To avoid confusion with the similarly named Limitable trait used elsewhere, alias here and use the alias in instanceof.
Import change:
-use WP_Ultimo\Models\Interfaces\Limitable; +use WP_Ultimo\Models\Interfaces\Limitable as LimitableInterface;Then, in handle_save() (Line 835), switch to:
if ($object instanceof LimitableInterface) { $object->handle_limitations(); }inc/models/interfaces/interface-notable.php (1)
17-55: Strengthen contracts with return type declarations (while keeping PHP 7.x compatibility)Limitable already uses return types for some methods (void). For consistency and better static analysis, add return types here where unambiguous.
Suggested diff:
-interface Notable { +interface Notable { @@ - public function get_notes(); + public function get_notes(): array; @@ - public function add_note($note); + public function add_note($note): bool; @@ - public function clear_notes(); + public function clear_notes(): bool; @@ - public function delete_note($note_id); + public function delete_note($note_id): bool; }Note: Parameter union types (array|\WP_Ultimo\Objects\Note) require PHP 8+. If you must support PHP 7.x, keep the param untyped and validate at runtime in implementations.
inc/managers/class-notes-manager.php (5)
15-16: Remove unused trait import
use WP_Ultimo\Models\Traits\Notable;is not used in this file. Drop it to avoid dead imports and keep phpcs clean.- use WP_Ultimo\Models\Traits\Notable;
112-118: Docblock type is too narrow for actual usageYou updated the param type to Notable, but inside the method we rely on
$obj->get_id()and$obj->model, which are not guaranteed by the Notable interface. This will confuse static analyzers and future readers.
- Either widen the phpdoc to an intersection that reflects reality (e.g., Notable & Base_Model), or
- Stop relying on the
modelpublic property and use an accessor guaranteed by a shared interface/base type.- * @param \WP_Ultimo\Models\Interfaces\Notable $obj The object. + * @param \WP_Ultimo\Models\Interfaces\Notable&\WP_Ultimo\Models\Base_Model $obj The object.If intersection types are undesirable in your toolchain, consider adding an inline
@phpstan-assertor switching to a method like$obj->get_model_slug()(if available) instead of$obj->model.
274-281: Prefer UUIDs over uniqid() for note IDs
uniqid()is not collision-resistant and depends on system time. WordPress provideswp_generate_uuid4()for robust unique IDs.- 'note_id' => uniqid(), + 'note_id' => wp_generate_uuid4(),
404-416: Use “Delete” terminology consistently in the delete-note modalThe delete-note modal currently mixes “clear”/“delete”, which is confusing. Align labels and copy with the action performed.
- 'title' => __('Confirm clear the note?', 'multisite-ultimate'), + 'title' => __('Confirm delete this note?', 'multisite-ultimate'), ... - 'title' => __('Clear Note', 'multisite-ultimate'), - 'placeholder' => __('Clear Note', 'multisite-ultimate'), + 'title' => __('Delete Note', 'multisite-ultimate'), + 'placeholder' => __('Delete Note', 'multisite-ultimate'),
268-298: Double-check capability and nonce enforcement for handlersYou set capabilities on the forms (“edit_notes”, “delete_notes”), which likely gate access before handlers run. If that’s not guaranteed in all entry paths, add an explicit
current_user_can()check and verify a nonce in each handler before mutating state.Would you like me to open a small follow-up PR that adds explicit capability and nonce checks to
handle_add_note_modal,handle_clear_notes_modal, andhandle_delete_note_modal?Also applies to: 365-393, 481-493
inc/objects/class-limitations.php (3)
362-366: Duplicate assignment can be removed (minor cleanup)There are two identical assignments to
$array1[$key]in sequence. Remove the duplicate to reduce noise. Not introduced in this PR, but easy to tidy up.- // Avoid change true values - $array1[ $key ] = true !== $original_value || ! $should_sum ? $value : true; - - $array1[ $key ] = true !== $original_value || ! $should_sum ? $value : true; + // Avoid change true values + $array1[ $key ] = true !== $original_value || ! $should_sum ? $value : true;
81-100: Optional: align exists() return type with phpdoc or adjust docblock
exists()currently returns the module instance orfalse, but phpdoc says@return bool. Either change the implementation to return a boolean or fix the docblock to reflect the actual return type (e.g.,bool|\WP_Ultimo\Limitations\Limit). Tests already rely on the current behavior.Also applies to: 159-172
59-60: Ensure PHP 7.4+ requirement and guard dynamicclass_exists()calls
• The project’s composer.json already requires PHP >= 7.4.1, so typed properties and__serialize/__unserializeare supported. If your WordPress plugin header still declares a lower PHP version under “Requires PHP,” please bump it to 7.4 to reflect this change.
• Multiple locations invokeclass_exists($variable)without first checking that the variable is a string. Passing a non-string toclass_exists()emits a PHP warning. Wrap these inis_string()to prevent warnings.Affected files and lines:
- inc/objects/class-limitations.php: lines 90–92 and 163–165
- inc/installers/class-migrator.php: lines 2581–2583
- inc/functions/gateway.php: lines 100–102 and 123–125
- inc/functions/rest.php: lines 25–27
- inc/functions/countries.php: lines 307–309
- inc/compat/class-general-compat.php: lines 456–457, 478–479, 503–504
- inc/class-ajax.php: lines 71–73
Example refactor:
- if (class_exists($class_name)) { + if (is_string($class_name) && class_exists($class_name)) {inc/models/interfaces/interface-billable.php (2)
12-12: Consistency nit: spacing around defined()Elsewhere in the codebase you use
defined('ABSPATH') || exit;. Consider aligning this line for consistency.-defined( 'ABSPATH' ) || exit; +defined('ABSPATH') || exit;
19-47: Consider explicit return/param types (when safe)If you plan a minor breaking bump, consider adding explicit return types to the interface to strengthen contracts:
get_default_billing_address(): \WP_Ultimo\Objects\Billing_Addressget_billing_address(): \WP_Ultimo\Objects\Billing_Addressset_billing_address(array|\WP_Ultimo\Objects\Billing_Address $billing_address): void(PHP 8 union)tests/WP_Ultimo/Objects/Limitations_Test.php (1)
146-159: Optional: add a test ensuring lazy __get uses raw_module_data when module wasn’t instantiatedTo assert the new
$raw_module_dataflow, you can unset an entry from$modulesvia reflection after constructing with data, then access it via__getand verify the instantiated module reflects the original raw data.If you want, I can draft this test method using reflection to unset a module and assert that
__get()hydrates it with$raw_module_data.Also applies to: 468-474
inc/models/class-payment.php (3)
658-681: Nit: “get_tax_breakthrough” likely meant “get_tax_breakdown”Purely naming. Consider renaming when you next make a BC-breaking pass.
901-942: Doc mismatch: get_invoice_number() declares int in doc but returns string in practiceBecause of prefixes and “(provisional)”, this method returns a string. Update the docblock to
@return string.- * @return int + * @return string
1030-1037: Doc types for refund() are misleading
$amountis annotated as boolean but is used as a numeric amount. Consider fixing the docblock tofloat|int|nulland$should_cancel_membership_on_refundtobool|null.- * @param boolean $amount The amount to refund. - * @param null|boolean $should_cancel_membership_on_refund If we should cancel a membership as well. + * @param float|int|null $amount The amount to refund (null/0 for full). + * @param bool|null $should_cancel_membership_on_refund If we should cancel a membership as well.views/dashboard-widgets/limits-and-quotas.php (1)
142-151: New extension hook is useful; document expected markup to avoid malformed lists.The hook fires inside a UL. Recommend documenting that callbacks should output one or more
elements (with appropriate classes) to keep the markup valid. Optionally, wrap the do_action in an output buffer and validate output if you want to enforce structure later. /** * Allow addons to add their own limit widgets. * * @since 2.4.4 * @param \WP_Ultimo\Models\Site $site Current site object. * @param array|false $items_to_display Items to display or false to show all. + * Developers: this action runs inside a <ul>. Please echo one or more <li>...</li> items. */ do_action('wu_dashboard_widget_limits_content', $site, $items_to_display);inc/managers/class-limitation-manager.php (3)
304-313: Toggle default value should be boolean, not integer.Field
modules[visits][enabled]uses'value' => 10. UI toggles normally expect boolean true/false. Set totrueor remove it to rely on computed state; otherwise v-model truthiness can be misleading.Apply this diff:
- 'value' => 10, + 'value' => true,
793-799: Method does not mutate $section; drop pass-by-reference to simplify.
get_theme_selection_listonly forwards$sectionto the template and doesn’t modify it. Removing by-ref reduces coupling and avoids the need for by-ref gymnastics at callers.Apply this diff:
- public function get_theme_selection_list($obj, &$section) { + public function get_theme_selection_list($obj, $section) {Note: If you adopt this, you can keep the closure from the previous comment but remove the local
$section =& ...indirection and pass$sections['allowed_themes']directly.
849-851: Minor naming nit: $all_plugins should be $all_themes.Variable name in
get_all_themes()is misleading. Rename for clarity.Apply this diff:
- $all_plugins = wp_get_themes(); - - return array_filter($all_plugins, fn($path) => ! in_array($path, $this->theme_exclusion_list(), true), ARRAY_FILTER_USE_KEY); + $all_themes = wp_get_themes(); + + return array_filter($all_themes, fn($path) => ! in_array($path, $this->theme_exclusion_list(), true), ARRAY_FILTER_USE_KEY);tests/WP_Ultimo/Managers/Payment_Manager_Test.php (6)
5-9: Remove unused import.
Customeris not used anywhere in this test.Apply this diff:
-use WP_Ultimo\Models\Customer;
49-72: Reflection unnecessary; call the public method directly.
invoice_viewer()is public. Also, clean up superglobals after the assertion.Apply this diff:
- $reflection = new \ReflectionClass($this->payment_manager); - $method = $reflection->getMethod('invoice_viewer'); - $method->setAccessible(true); + // Call directly; method is public. // The method should pass nonce validation but fail on payment lookup // This confirms that our nonce validation logic is working correctly $this->expectException(\WPDieException::class); $this->expectExceptionMessage('This invoice does not exist.'); - $method->invoke($this->payment_manager); + $this->payment_manager->invoice_viewer();
100-124: Same as above: drop reflection; direct call suffices.Keeps the test simpler and less brittle.
Apply this diff:
- $reflection = new \ReflectionClass($this->payment_manager); - $method = $reflection->getMethod('invoice_viewer'); - $method->setAccessible(true); + // Call directly; method is public. @@ - $method->invoke($this->payment_manager); + $this->payment_manager->invoice_viewer();
129-147: Same: avoid reflection; assert early return without output.Direct invocation is fine.
Apply this diff:
- $reflection = new \ReflectionClass($this->payment_manager); - $method = $reflection->getMethod('invoice_viewer'); - $method->setAccessible(true); @@ - $method->invoke($this->payment_manager); + $this->payment_manager->invoice_viewer();
152-170: Same: avoid reflection for missing reference test.Keep tests uniform and simpler.
Apply this diff:
- $reflection = new \ReflectionClass($this->payment_manager); - $method = $reflection->getMethod('invoice_viewer'); - $method->setAccessible(true); @@ - $method->invoke($this->payment_manager); + $this->payment_manager->invoice_viewer();
175-193: Same + avoid reliance on get_hash(); any non-empty reference works if you need it later.Here you don’t set
key, so presence check will short-circuit anyway. If you want to be explicit, you can setreference = 'dummy'.Apply this diff:
- $_REQUEST['reference'] = self::$payment->get_hash(); + $_REQUEST['reference'] = 'dummy_reference'; @@ - $reflection = new \ReflectionClass($this->payment_manager); - $method = $reflection->getMethod('invoice_viewer'); - $method->setAccessible(true); + // Call directly; method is public. @@ - $method->invoke($this->payment_manager); + $this->payment_manager->invoice_viewer();lang/multisite-ultimate.pot (11)
8816-8826: Tighten wording and match WP capability naming ("Edit Users").These UI strings read slightly awkward and diverge from WordPress’ “Users → Edit Users” wording. Recommend small grammar/style fixes for clarity and consistency.
Apply this diff to the msgids (source strings) and re-generate the POT:
-Enable Edit User Capability +Enable "Edit Users" Capability -Allow site owners to edit the user accounts of users on their own site. Also lets site owners create user accounts without requiring email confirmation. +Allow site owners to edit user accounts on their own sites. Also lets site owners create user accounts without requiring email confirmation. -Add the user without sending an email that requires their confirmation +Add the user without sending a confirmation email
14969-14971: Prefer “email address” for clarity.“The %s is not a valid email” is better as “email address.” Broadly used elsewhere in this file too.
-msgid "The %s is not a valid email" +msgid "The %s is not a valid email address"
15223-15235: Inconsistent naming: “Conversational Fluent Forms” vs “Fluent Forms” vs “Conversational Forms”.These four strings mix three labels for what appears to be the same feature. Pick one canonical label (e.g., “Conversational Forms”) and use it consistently across limits/warnings to avoid user confusion and reduce translator effort.
-msgid "Your plan does not support creating Conversational Fluent Forms." +msgid "Your plan does not support creating Conversational Forms." -msgid "Your plan does not support creating Fluent Forms." +msgid "Your plan does not support creating Conversational Forms." -msgid "You have reached the limit of Conversational Fluent Forms. Please upgrade your plan." +msgid "You have reached the limit of Conversational Forms. Please upgrade your plan." -msgid "You have reached the limit of Fluent Forms. Please upgrade your plan." +msgid "You have reached the limit of Conversational Forms. Please upgrade your plan."Follow-up: if “Conversational Forms” is a product/feature name rather than a generic description, consider adding a translator note in the source to keep capitalization/brand intact.
17306-17323: Good move to ordered placeholders; add translator guidance.Nice switch to ordered placeholders in “The customer will be able to create %1$s form(s) of this type. %2$s”. Consider a translator note in the source clarifying:
- %1$s is a number (or “unlimited”)
- %2$s is an optional trailing sentence/tip
1912-1913: Use “cannot,” not “can not.”Micro grammar fix; also ensure this change propagates to all occurrences of the same string.
-msgid "This action can not be undone." +msgid "This action cannot be undone."
20101-20112: Invoice labels: improve localizability and consistency.
- “Invoice #” can be hard to localize; prefer “Invoice No.” or split the label from the value.
- “Created: %s” → “Created on: %s”
- “Due on Receipt” → “Due upon receipt”
-msgid "Invoice #" +msgid "Invoice No." -msgid "Created: %s" +msgid "Created on: %s" -msgid "Due on Receipt" +msgid "Due upon receipt"If “Invoice #” is rendered alongside a dynamic number, an even better approach is two elements: a static “Invoice” label and a separate number field, letting translators control order/layout.
Also applies to: 20131-20131
8734-8739: Sunrise messages OK; consider punctuation/tense alignment.Both messages read well and end with punctuation; no action strictly needed. If you want perfect symmetry: “Sunrise upgrade succeeded.” (shorter, same tense/voice as the failure string).
17669-17684: Payment status phrasing and context.
- “Full Refund” / “Partial Refund” might be nouns (status) or actions (verbs). Consider adding translator comments in source to lock intended part-of-speech. If they’re statuses, “Fully Refunded” / “Partially Refunded” may be clearer.
- “Processed on %s.” looks good; leave as-is.
Potential tweaks if these are statuses:
-msgid "Full Refund" +msgid "Fully Refunded" -msgid "Partial Refund" +msgid "Partially Refunded"
1776-1776: Shared commerce labels OK; consider translator notes for units/context.“Discount,” “Total,” “Tax,” “Price,” and “Payment Method” are common strings used many times. If some usages imply different contexts (table header vs. button vs. narrative), consider adding translator comments in source where ambiguity exists.
Also applies to: 3662-3662, 3833-3833, 3947-3947, 7810-7810
20101-20131: One more i18n best-practice: avoid embedded symbols when possible.If feasible, avoid symbols (like “#”) in labels; they don’t translate well in some locales. Using words (“No.”/“Number”) improves clarity.
8816-8826: Quick string‐consistency suggestions• lang/multisite-ultimate.pot, line 1912
– Change
msgid "This action can not be undone."
to
msgid "This action cannot be undone."• lang/multisite-ultimate.pot, lines 15723/15727/15731/15735/17215/17223/17307
– Several variants of the “Fluent Forms” label appear (“Conversational Fluent Forms”, “Fluent Forms”, “Conversational Forms”). Pick one canonical term (e.g. “Conversational Fluent Forms”) and update all occurrences to reduce translator churn.• lang/multisite-ultimate.pot, line 14969
– Change
msgid "The %s is not a valid email"
to
msgid "The %s is not a valid email address"• lang/multisite-ultimate.pot, line 20101
– Change
msgid "Invoice #"
to
msgid "Invoice No."
(or split into separate “Invoice” label + “#” value fields)Also apply the same updates in the other affected line ranges (14969–14971, 15223–15235, 1912–1913, 20101–20112, 20131–20131) to keep translations consistent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
assets/js/lib/apexcharts.min.jsis excluded by!**/*.min.jsassets/js/lib/shepherd.min.jsis excluded by!**/*.min.js
📒 Files selected for processing (22)
README.md(1 hunks)assets/js/lib/apexcharts.js(31 hunks)assets/js/lib/shepherd.js(20 hunks)inc/admin-pages/class-edit-admin-page.php(1 hunks)inc/class-sunrise.php(2 hunks)inc/managers/class-limitation-manager.php(8 hunks)inc/managers/class-notes-manager.php(2 hunks)inc/models/class-customer.php(2 hunks)inc/models/class-membership.php(2 hunks)inc/models/class-payment.php(2 hunks)inc/models/class-product.php(1 hunks)inc/models/class-site.php(2 hunks)inc/models/interfaces/interface-billable.php(1 hunks)inc/models/interfaces/interface-limitable.php(1 hunks)inc/models/interfaces/interface-notable.php(1 hunks)inc/objects/class-limitations.php(6 hunks)lang/multisite-ultimate.pot(20 hunks)package.json(1 hunks)tests/WP_Ultimo/Managers/Payment_Manager_Test.php(1 hunks)tests/WP_Ultimo/Objects/Limitations_Test.php(1 hunks)tests/WP_Ultimo/Sunrise_Test.php(1 hunks)views/dashboard-widgets/limits-and-quotas.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
inc/models/class-customer.php (2)
inc/models/class-base-model.php (1)
Base_Model(27-1182)inc/admin-pages/class-customer-edit-admin-page.php (1)
Customer_Edit_Admin_Page(20-1252)
tests/WP_Ultimo/Sunrise_Test.php (1)
inc/class-sunrise.php (13)
Sunrise(22-467)should_startup(91-98)should_load_sunrise(384-389)maybe_tap(412-425)maybe_tap_on_init(397-402)system_info(333-376)init(45-77)load(191-234)loaded(248-251)load_dependencies(106-161)load_domain_mapping(169-183)manage_sunrise_updates(259-268)try_upgrade(277-294)
inc/models/interfaces/interface-billable.php (3)
inc/models/class-customer.php (1)
get_default_billing_address(221-230)inc/models/class-membership.php (1)
get_default_billing_address(1503-1514)inc/admin-pages/customer-panel/class-account-admin-page.php (1)
Account_Admin_Page(20-254)
views/dashboard-widgets/limits-and-quotas.php (1)
inc/admin-pages/class-dashboard-admin-page.php (2)
hooks(117-123)output_widget_new_accounts(423-479)
tests/WP_Ultimo/Objects/Limitations_Test.php (1)
inc/objects/class-limitations.php (14)
Limitations(24-508)__serialize(109-112)__unserialize(123-125)build_modules(135-148)build(159-172)exists(182-185)has_limitations(193-204)is_module_enabled(214-219)merge(235-259)to_array(376-379)early_get_limitations(390-432)remove_limitations(443-464)get_empty(472-481)repository(491-507)
tests/WP_Ultimo/Managers/Payment_Manager_Test.php (3)
inc/models/class-payment.php (1)
Payment(28-1133)inc/invoices/class-invoice.php (1)
Invoice(23-304)inc/models/class-base-model.php (2)
get_hash(181-192)query(833-842)
inc/managers/class-limitation-manager.php (1)
inc/objects/class-limitations.php (1)
Limitations(24-508)
inc/models/class-site.php (1)
inc/models/class-base-model.php (1)
Base_Model(27-1182)
assets/js/lib/shepherd.js (1)
assets/js/lib/shepherd.min.js (16)
y(1-1)a(1-1)a(1-1)e(1-1)e(1-1)e(1-1)e(1-1)e(1-1)e(1-1)e(1-1)e(1-1)e(1-1)e(1-1)e(1-1)e(1-1)e(1-1)
🪛 GitHub Actions: Unit & Integration Tests
tests/WP_Ultimo/Sunrise_Test.php
[error] 74-74: WP_Ultimo\Sunrise_Test::test_system_info_no_fatal_errors failed: gmdate() expects parameter 2 to be int, string given
[error] 93-93: WP_Ultimo\Sunrise_Test::test_system_info_adds_sunrise_data failed: gmdate() expects parameter 2 to be int, string given
inc/class-sunrise.php
[error] 354-354: gmdate() expects parameter 2 to be int, string given
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: cypress (8.3, chrome)
- GitHub Check: cypress (8.3, firefox)
- GitHub Check: cypress (7.4, firefox)
- GitHub Check: cypress (8.4, chrome)
- GitHub Check: cypress (7.4, chrome)
- GitHub Check: cypress (8.4, firefox)
- GitHub Check: cypress (8.2, chrome)
- GitHub Check: cypress (8.1, firefox)
- GitHub Check: cypress (8.0, chrome)
- GitHub Check: cypress (8.0, firefox)
- GitHub Check: cypress (8.2, firefox)
- GitHub Check: cypress (8.1, chrome)
🔇 Additional comments (57)
assets/js/lib/shepherd.js (10)
1-1: Vendor bump to Shepherd 14.5.1 — LGTMThe version header reflects the intended bump. No issues spotted with the packaging of this vendor asset in the repo structure.
1276-1290: flip(): smarter cross-axis handling — verify no regressionsThe new gating around ignoreCrossAxisOverflow vs initial main-axis overflow looks correct and should reduce unnecessary flips. Since this alters placement iteration order, please sanity-check common placements in both LTR and RTL pages (with constrained containers) to ensure no regressions.
Run these quick checks manually:
- Tooltip attached on each side (top/bottom/left/right), with and without alignments (-start/-end), while forcing overflow via small containers.
- Confirm next placement is attempted only when the initial main-axis overflow is positive or we’re not ignoring cross-axis overflow.
1402-1406: shift(): exposes enabled map in middlewareDataNew data.enabled flags for mainAxis/crossAxis are added. This is fine and backwards-compatible for consumers that ignore unknown fields. If any downstream code introspects middlewareData.shift, validate it doesn’t assume a fixed shape.
1550-1552: Containing block detection broadened — cross-browser checkExpanding isContainingBlock to consider individual transforms, containerType, backdropFilter, filter, willChange, and contain improves correctness but increases reliance on CSSOM support. Safari/WebKit detection is considered via isWebKit(), but please verify in:
- Safari 15+ and iOS Safari.
- Firefox with various contain/will-change combinations.
1993-2012: RTL scrollbar offset handling: new helper and broader applicationIntroducing setLeftRTLScrollbarOffset() and applying it for fixed and non-offset-parent scenarios should fix edge cases (e.g., Firefox with left-side scrollbars). Edge-case ask:
- Validate in RTL documents with body scrollbar left (Firefox layout.scrollbar.side=3) for both position: fixed and non-fixed tooltips.
- Check that LTR pages don’t get a spurious offset.
2100-2102: rectsAreEqual(): utility addition looks goodSimple value equality on x, y, width, height. A small but useful helper for reducing noisy updates.
2123-2171: IntersectionObserver: reduce false positives on ratio==1 — nice robustness improvementCapturing the element rect pre-observer and re-checking after notifications mitigates browser IO quirks. Looks good. Keep an eye on potential extra refresh() calls under performance constraints; nothing blocking.
2248-2253: autoUpdate(): fewer spurious updatesUsing rectsAreEqual to guard updates in the rAF loop is a solid optimization.
2747-2760: New onDestroy(fn) in Svelte runtime — good additionThe public-like onDestroy hook is useful, especially for SSR scenarios. No issues.
3860-3871: Heads-up (pre-existing): innerHTML usage for title/text is potentially dangerousWhile not introduced in this diff, both Shepherd_title and Shepherd_text set innerHTML from options. If any title/text originates from user input, ensure upstream sanitization to avoid XSS.
Would you like a small utility to sanitize/escape strings before passing them to Shepherd?
assets/js/lib/apexcharts.js (21)
12259-12262: Newchart.injectStyleSheetdefault looks goodDefaulting to
trueand subsequently gating injections off this flag is sane.
12350-12353: Global parsing config is a useful addition—ensure it’s documentedNew
parsing: { x, y }opens a clean path to map raw objects. Make sure README/API docs cover precedence (series-level over global) and supported paths.
14851-14855: Globals for parsing state initialized correctlyThe added globals (
dataWasParsed,originalSeries) set a clear contract for parse/reset cycles.
17241-17251: Label array build logic unchanged—LGTM
17261-17267: Duplication for buildinggl.seriesXis fine
17430-17433: Parsing is invoked early—correct integration pointCalling
parseRawDataIfNeededbefore cloning intoinitialSeriesis right; keepsinitialSeriesaligned with parsed structure.
17541-17556: Conditional legend styles on export—LGTMIncluding legend CSS only when visible avoids unnecessary bloat in exported SVGs.
17947-17953: Border width coercion to string before percentage check—LGTM
19345-19349: xAxis scale respects configured max—LGTM
21892-21897: Legend stylesheet node creation—LGTMCreating a single text node from the CSS string is fine.
21913-21919: Respectchart.injectStyleSheetin FO injection—LGTM
22501-22507: Toolbar icon classes simplified—LGTM
22512-22518: Pan control class simplification—LGTM
34038-34049: Parsing flags preserved across global re-init—LGTMCapturing and restoring
dataWasParsedandoriginalSeriesavoids losing parsing state duringGlobalsre-init.
34241-34248: Reset+reparse on_updateSerieslooks correctResetting parsing flags before
parseData(newSeries)ensures updates reflect new parsing rules.
35909-35915: RespectinjectStyleSheetin Shadow DOM fallback—LGTM
36199-36203: Reset parsing flags onupdateOptions({ series })—LGTMPrevents stale
__apexParsedfrom short-circuiting re-parsing.
36237-36243:updateSeriesresets parsing and axis bounds—LGTM
36253-36259:appendSeriesresets parsing before merge—LGTM
36271-36277:appendDataresets parsing and captures previous paths—LGTM
33795-33802: No action needed —seris properly defined as a function parameterThe
seridentifier used on line 33797 is the first parameter ofplotChartType(ser, xyRatios), so it’s already declared in the enclosing scope and not undefined.– Resolved: the reference to
ser[st]is valid within the callback.inc/models/interfaces/interface-limitable.php (1)
9-9: All legacy references removed and interface autoload verified
- No occurrences of the old
WP_Ultimo\Models\Limitablenamespace remain.- inc/class-sunrise.php now explicitly requires
models/interfaces/interface-limitable.php(line 118), ensuring theLimitableinterface is defined before any checks.- All call sites import and use the new interface FQCN:
- inc/models/class-site.php:
use WP_Ultimo\Models\Interfaces\Limitable;andimplements Limitable- inc/models/class-product.php:
use WP_Ultimo\Models\Interfaces\Limitable;andimplements Limitable- inc/models/class-membership.php:
use WP_Ultimo\Models\Interfaces\Limitable;andimplements Limitable- inc/admin-pages/class-edit-admin-page.php:
use WP_Ultimo\Models\Interfaces\Limitable;withinstanceof LimitablechecksAll autoload and call–site updates for the moved interface are in place.
inc/models/interfaces/interface-notable.php (2)
10-12: Namespace and ABSPATH guard look good
The interface is correctly namespaced under WP_Ultimo\Models\Interfaces and guarded with ABSPATH. No issues here.
32-34: The script above will extract theadd_notedocblock in the trait so we can confirm whether its@parammatches the interface (and decide if it, too, needs updating). Once we have that, I’ll provide the precise diff for both interface and—if necessary—the trait.inc/objects/class-limitations.php (1)
137-138: Good: persist raw module data for lazy instantiationStoring
$modules_datainto$this->raw_module_dataenables consistent data when a module is first accessed via__get. This is the right direction.tests/WP_Ultimo/Objects/Limitations_Test.php (4)
93-112: Test calls __get with a non-existent module — make sure production code can’t fatally error on PHP 8+The test expects a graceful
falsefornon_existent_module. Without theis_string(...) && class_exists(...)guard inLimitations::__get, this call can throw a TypeError on PHP 8+. Ensure the guard lands with this PR so the test remains stable across runtimes.Also applies to: 119-129
166-187: Nice coverage on build_modules pathsThe data provider covers valid, empty, and mixed scenarios, asserting internal state via reflection. Looks good.
Also applies to: 194-207
475-502: Good isolation of DB interactions with wpdb mocks and cache coverageThe tests for early_get_limitations/remove_limitations and caching behavior are thorough and isolated. This will help prevent regressions around SQL composition and caching.
Also applies to: 507-529, 534-552, 700-724
600-631: Behavior/visibility priority and force-enabled branches well coveredGreat to see direct tests for the tricky merge semantics; this has bitten in the past. No action needed.
Also applies to: 636-655, 659-672, 677-695
inc/models/class-payment.php (1)
13-14: Payment correctly implements NotableImporting
Interfaces\Notableand applyingTraits\Notablemakes Payments compatible with the new notes UI (filters are already hooked in Notes_Manager). No runtime issues spotted.Also applies to: 28-31
inc/models/class-customer.php (1)
13-14: Interfaces + traits adoption looks correct; verify contracts are fully satisfied.Customer now implements Interfaces\Billable and Interfaces\Notable and pulls in the corresponding Traits. The class already provides get_default_billing_address(), so Billable is covered; Notable methods are provided by the trait. No backward-incompatible signature changes detected.
If the Customer model can be instantiated outside the Sunrise early-boot path, confirm that your autoloader loads the interfaces/traits (or that they’re conditionally required elsewhere) to avoid "interface not found" fatals.
Also applies to: 27-31
inc/models/class-membership.php (1)
13-17: Additive public surface (Billable + Notable) is sensible; traits wired properly.Membership now implements Limitable, Billable, Notable, and uses the matching traits. This should be non-breaking for consumers and unblocks note/billing extensions on memberships.
Please confirm there are no method name collisions introduced by Traits\Notable or Traits\Billable with existing Membership methods.
Also applies to: 26-32
inc/class-sunrise.php (1)
118-121: Early-loading new interfaces is appropriate for Sunrise context.Requiring the new interface files here is the right move given Sunrise runs pre–full-autoload. This avoids class/interface-not-found errors when models initialize early.
views/dashboard-widgets/limits-and-quotas.php (1)
7-7: No-op formatting tweak is fine.tests/WP_Ultimo/Sunrise_Test.php (1)
68-82: Tests are fine; they exposed the real bug.Catching TypeError around system_info was pragmatic. With the Sunrise fix (int defaults + casts), these branches should now pass in the “no exception” path. No test changes required.
After applying the Sunrise fixes, please re-run the suite to confirm both failing tests pass.
Also applies to: 87-114
inc/managers/class-limitation-manager.php (7)
19-20: Import reorder/retarget looks fine.Keeping
WP_Ultimo\Objects\Limitationsimported (used at Line 211) is necessary; no runtime impact from the reordering.
226-235: Docblock: type tightened to Interfaces\Limitable — good.Clearer parameter type improves IDE/static analysis without changing runtime.
252-257: Docblock updates for add_limitation_sections — good.Parameters and return typing are clearer and consistent with the rest of the file.
572-579: Docblock updates for register_user_fields — good.Consistent typing and description; matches usage.
639-644: Docblock updates for register_post_type_fields — good.Accurate and consistent with method semantics.
709-717: Docblock updates for get_sites_fields — good.Makes the contract explicit; helps consumers and tests.
772-776: Docblock for get_plugin_selection_list — good.Return type and param match implementation.
inc/models/class-site.php (2)
14-16: Interfaces imported explicitly — good modernization.Importing
LimitableandNotableimproves type clarity and aligns with the broader PR.
26-31: Implements Limitable and Notable; traits applied — LGTM.The class advertises capabilities formally while the traits likely supply the implementations. Watch for method-name collisions between traits.
lang/multisite-ultimate.pot (3)
12-12: POT metadata refresh looks good.POT-Creation-Date bumped to 2025-08-22T02:48:06+00:00. No issues.
1342-1344: Reference additions for “None” look fine.New source references added without altering the msgid. No concerns.
17306-17308: UI label “Conversational Forms” aligns with earlier rename.This matches the suggested single canonical label. Keep it consistent across all UI surfaces.
| /*! | ||
| * ApexCharts v5.0.0 | ||
| * ApexCharts v5.3.3 | ||
| * (c) 2018-2025 ApexCharts | ||
| * Released under the MIT License. | ||
| */ | ||
| (function (global, factory) { | ||
| typeof exports === 'object' && typeof module !== 'undefined' ? module.exports = factory() : |
There was a problem hiding this comment.
Restore third‑party license header for compliance
The MIT license line appears to have been removed from the vendor bundle header. Distributing MIT-licensed code without preserving the license notice is a compliance risk.
Apply this minimal fix to reinstate the notice:
/*!
- * ApexCharts v5.3.3
+ * ApexCharts v5.3.3
* (c) 2018-2025 ApexCharts
+ * Released under the MIT License
+ * See LICENSE file or https://github.com/apexcharts/apexcharts.js/blob/master/LICENSE
*/📝 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.
| /*! | |
| * ApexCharts v5.0.0 | |
| * ApexCharts v5.3.3 | |
| * (c) 2018-2025 ApexCharts | |
| * Released under the MIT License. | |
| */ | |
| (function (global, factory) { | |
| typeof exports === 'object' && typeof module !== 'undefined' ? module.exports = factory() : | |
| /*! | |
| * ApexCharts v5.3.3 | |
| * (c) 2018-2025 ApexCharts | |
| * Released under the MIT License | |
| * See LICENSE file or https://github.com/apexcharts/apexcharts.js/blob/master/LICENSE | |
| */ |
🤖 Prompt for AI Agents
In assets/js/lib/apexcharts.js lines 1 to 6, the vendor bundle header is missing
the MIT license notice; restore the original third-party license header by
re-adding the MIT license line(s) exactly as they appeared in the upstream
distribution (author, copyright years, and "MIT License" or the short MIT
notice), placing them at the top of the file within the existing comment block
so the full license attribution is preserved.
| key: "parseNumber", | ||
| value: function parseNumber(val) { | ||
| if (val === null) return val; | ||
| if (typeof val === 'number' || val === null) return val; | ||
| return parseFloat(val); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden number parsing to avoid NaN propagation and treat empty/undefined as null
Current logic returns parseFloat(val) for any non-number, which can produce NaN and treats empty strings as 0 inconsistently. Recommend normalizing to null for empty/invalid inputs and guarding NaN.
- if (typeof val === 'number' || val === null) return val;
- return parseFloat(val);
+ if (val === null || val === undefined) return null;
+ if (typeof val === 'number') return Number.isFinite(val) ? val : null;
+ if (typeof val === 'string') {
+ var trimmed = val.trim();
+ if (trimmed === '') return null;
+ var n = Number(trimmed);
+ return Number.isFinite(n) ? n : null;
+ }
+ var n2 = Number(val);
+ return Number.isFinite(n2) ? n2 : null;📝 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.
| key: "parseNumber", | |
| value: function parseNumber(val) { | |
| if (val === null) return val; | |
| if (typeof val === 'number' || val === null) return val; | |
| return parseFloat(val); | |
| } | |
| if (val === null || val === undefined) return null; | |
| if (typeof val === 'number') return Number.isFinite(val) ? val : null; | |
| if (typeof val === 'string') { | |
| var trimmed = val.trim(); | |
| if (trimmed === '') return null; | |
| var n = Number(trimmed); | |
| return Number.isFinite(n) ? n : null; | |
| } | |
| var n2 = Number(val); | |
| return Number.isFinite(n2) ? n2 : null; |
🤖 Prompt for AI Agents
In assets/js/lib/apexcharts.js around lines 421 to 425, the current parsing
returns parseFloat(val) for non-number inputs which can yield NaN and treats
empty strings inconsistently; change the logic to: treat
null/undefined/empty-string (after trimming) as null, return the value unchanged
if it's already a number, otherwise attempt parseFloat on the trimmed string and
if the result is NaN return null, else return the parsed number — this prevents
NaN propagation and normalizes empty/invalid inputs to null.
| value: function parseDataNonAxisCharts(ser) { | ||
| var gl = this.w.globals; | ||
| var cnf = this.w.config; | ||
| gl.series = ser.slice(); | ||
| gl.seriesNames = cnf.labels.slice(); | ||
| for (var i = 0; i < gl.series.length; i++) { | ||
| if (gl.seriesNames[i] === undefined) { | ||
| gl.seriesNames.push('series-' + (i + 1)); | ||
|
|
||
| // Check if we have both old format (numeric series + labels) and new format | ||
| var hasOldFormat = Array.isArray(ser) && ser.every(function (s) { | ||
| return typeof s === 'number'; | ||
| }) && cnf.labels.length > 0; | ||
| var hasNewFormat = Array.isArray(ser) && ser.some(function (s) { | ||
| return s && _typeof(s) === 'object' && s.data || s && _typeof(s) === 'object' && s.parsing; | ||
| }); | ||
| if (hasOldFormat && hasNewFormat) { | ||
| console.warn('ApexCharts: Both old format (numeric series + labels) and new format (series objects with data/parsing) detected. Using old format for backward compatibility.'); | ||
| } | ||
|
|
||
| // If old format exists, use it (backward compatibility priority) | ||
| if (hasOldFormat) { | ||
| gl.series = ser.slice(); | ||
| gl.seriesNames = cnf.labels.slice(); | ||
| for (var i = 0; i < gl.series.length; i++) { | ||
| if (gl.seriesNames[i] === undefined) { | ||
| gl.seriesNames.push('series-' + (i + 1)); | ||
| } | ||
| } | ||
| return this.w; | ||
| } | ||
|
|
||
| // Check if it's just a plain numeric array without labels (radialBar common case) | ||
| if (Array.isArray(ser) && ser.every(function (s) { | ||
| return typeof s === 'number'; | ||
| })) { | ||
| gl.series = ser.slice(); | ||
| gl.seriesNames = []; | ||
| for (var _i = 0; _i < gl.series.length; _i++) { | ||
| gl.seriesNames.push(cnf.labels[_i] || "series-".concat(_i + 1)); | ||
| } | ||
| return this.w; | ||
| } | ||
| var processedData = this.extractPieDataFromSeries(ser); | ||
| gl.series = processedData.values; | ||
| gl.seriesNames = processedData.labels; | ||
|
|
||
| // Special handling for radialBar - ensure percentages are valid | ||
| if (cnf.chart.type === 'radialBar') { | ||
| gl.series = gl.series.map(function (val) { | ||
| var numVal = Utils$1.parseNumber(val); | ||
| if (numVal > 100) { | ||
| console.warn("ApexCharts: RadialBar value ".concat(numVal, " > 100, consider using percentage values (0-100)")); | ||
| } | ||
| return numVal; | ||
| }); | ||
| } | ||
|
|
||
| // Ensure we have proper fallback names | ||
| for (var _i2 = 0; _i2 < gl.series.length; _i2++) { | ||
| if (gl.seriesNames[_i2] === undefined) { | ||
| gl.seriesNames.push('series-' + (_i2 + 1)); | ||
| } | ||
| } | ||
| return this.w; | ||
| } |
There was a problem hiding this comment.
Non-axis parsing: support numeric strings and avoid hard failures for legacy inputs
Two potential regressions:
- Old-format detection requires strict numbers; numeric strings (e.g.,
["10","20"]) won’t match and will fall through to the “unsupported” branch. - Plain numeric array path also rejects numeric strings.
Treat numeric strings as numeric for backward compatibility.
Apply these changes:
- var hasOldFormat = Array.isArray(ser) && ser.every(function (s) {
- return typeof s === 'number';
- }) && cnf.labels.length > 0;
+ var hasOldFormat = Array.isArray(ser) && ser.every(function (s) {
+ return (typeof s === 'number' && Number.isFinite(s)) ||
+ (typeof s === 'string' && s.trim() !== '' && !isNaN(Number(s)));
+ }) && cnf.labels.length > 0;
...
- if (Array.isArray(ser) && ser.every(function (s) {
- return typeof s === 'number';
- })) {
- gl.series = ser.slice();
+ if (Array.isArray(ser) && ser.every(function (s) {
+ return (typeof s === 'number' && Number.isFinite(s)) ||
+ (typeof s === 'string' && s.trim() !== '' && !isNaN(Number(s)));
+ })) {
+ gl.series = ser.map(function (s) { return Utils$1.parseNumber(s); });Also consider filtering/normalizing NaN to null as per the parseNumber hardening above.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In assets/js/lib/apexcharts.js around lines 17031 to 17091, the non-axis parsing
paths reject numeric strings and treat them as unsupported (both the old-format
detection and the plain numeric array branch); update those branches to coerce
numeric strings to numbers (e.g., via the existing parseNumber helper) so arrays
like ["10","20"] are accepted, and ensure after coercion any NaN results are
normalized to null (filter/replace NaN -> null) before returning/using the
values to preserve backward compatibility and match parseNumber hardening.
| }, { | ||
| key: "parseRawDataIfNeeded", | ||
| value: function parseRawDataIfNeeded(series) { | ||
| var _this2 = this; | ||
| var cnf = this.w.config; | ||
| var gl = this.w.globals; | ||
| var globalParsing = cnf.parsing; | ||
|
|
||
| // If data was already parsed, don't parse again | ||
| if (gl.dataWasParsed) { | ||
| return series; | ||
| } | ||
|
|
||
| // If no global parsing config and no series-level parsing, return as-is | ||
| if (!globalParsing && !series.some(function (s) { | ||
| return s.parsing; | ||
| })) { | ||
| return series; | ||
| } | ||
| var processedSeries = series.map(function (serie, index) { | ||
| var _serie$parsing, _serie$parsing2, _serie$parsing3; | ||
| if (!serie.data || !Array.isArray(serie.data) || serie.data.length === 0) { | ||
| return serie; | ||
| } | ||
|
|
||
| // Resolve effective parsing config for this series | ||
| var effectiveParsing = { | ||
| x: ((_serie$parsing = serie.parsing) === null || _serie$parsing === void 0 ? void 0 : _serie$parsing.x) || (globalParsing === null || globalParsing === void 0 ? void 0 : globalParsing.x), | ||
| y: ((_serie$parsing2 = serie.parsing) === null || _serie$parsing2 === void 0 ? void 0 : _serie$parsing2.y) || (globalParsing === null || globalParsing === void 0 ? void 0 : globalParsing.y), | ||
| z: ((_serie$parsing3 = serie.parsing) === null || _serie$parsing3 === void 0 ? void 0 : _serie$parsing3.z) || (globalParsing === null || globalParsing === void 0 ? void 0 : globalParsing.z) | ||
| }; | ||
|
|
||
| // If no effective parsing config, return as-is | ||
| if (!effectiveParsing.x && !effectiveParsing.y) { | ||
| return serie; | ||
| } | ||
|
|
||
| // Check if data is already in {x, y} format or 2D array format | ||
| var firstDataPoint = serie.data[0]; | ||
| if (_typeof(firstDataPoint) === 'object' && firstDataPoint !== null && (firstDataPoint.hasOwnProperty('x') || firstDataPoint.hasOwnProperty('y')) || Array.isArray(firstDataPoint)) { | ||
| return serie; | ||
| } | ||
|
|
||
| // Validate that we have both x and y parsing config | ||
| if (!effectiveParsing.x || !effectiveParsing.y || Array.isArray(effectiveParsing.y) && effectiveParsing.y.length === 0) { | ||
| console.warn("ApexCharts: Series ".concat(index, " has parsing config but missing x or y field specification")); | ||
| return serie; | ||
| } | ||
|
|
||
| // Transform raw data to {x, y} format | ||
| var transformedData = serie.data.map(function (item, itemIndex) { | ||
| if (_typeof(item) !== 'object' || item === null) { | ||
| console.warn("ApexCharts: Series ".concat(index, ", data point ").concat(itemIndex, " is not an object, skipping parsing")); | ||
| return item; | ||
| } | ||
| var x = _this2.getNestedValue(item, effectiveParsing.x); | ||
| var y; | ||
| var z = undefined; | ||
| if (Array.isArray(effectiveParsing.y)) { | ||
| var yValues = effectiveParsing.y.map(function (fieldName) { | ||
| return _this2.getNestedValue(item, fieldName); | ||
| }); | ||
| if (_this2.w.config.chart.type === 'bubble' && yValues.length === 2) { | ||
| // For bubble: [y-value, z-value] → y = yValues[0], z = yValues[1] | ||
| y = yValues[0]; | ||
| } else { | ||
| y = yValues; | ||
| } | ||
| } else { | ||
| y = _this2.getNestedValue(item, effectiveParsing.y); | ||
| } | ||
|
|
||
| // explicit z field for bubble charts | ||
| if (effectiveParsing.z) { | ||
| z = _this2.getNestedValue(item, effectiveParsing.z); | ||
| } | ||
|
|
||
| // Warn if fields don't exist | ||
| if (x === undefined) { | ||
| console.warn("ApexCharts: Series ".concat(index, ", data point ").concat(itemIndex, " missing field '").concat(effectiveParsing.x, "'")); | ||
| } | ||
| if (y === undefined) { | ||
| console.warn("ApexCharts: Series ".concat(index, ", data point ").concat(itemIndex, " missing field '").concat(effectiveParsing.y, "'")); | ||
| } | ||
| var result = { | ||
| x: x, | ||
| y: y | ||
| }; | ||
| if (_this2.w.config.chart.type === 'bubble' && Array.isArray(effectiveParsing.y) && effectiveParsing.y.length === 2) { | ||
| var zValue = _this2.getNestedValue(item, effectiveParsing.y[1]); | ||
| if (zValue !== undefined) { | ||
| result.z = zValue; | ||
| } | ||
| } | ||
| if (z !== undefined) { | ||
| result.z = z; | ||
| } | ||
| return result; | ||
| }); | ||
| return _objectSpread2(_objectSpread2({}, serie), {}, { | ||
| data: transformedData, | ||
| __apexParsed: true | ||
| }); | ||
| }); | ||
|
|
||
| // Mark that data was parsed | ||
| gl.dataWasParsed = true; | ||
| if (!gl.originalSeries) { | ||
| gl.originalSeries = Utils$1.clone(series); | ||
| } | ||
| return processedSeries; | ||
| } | ||
|
|
||
| /** | ||
| * Get nested object value using dot notation path | ||
| * @param {Object} obj - The object to search in | ||
| * @param {string} path - Dot notation path (e.g., 'user.profile.name') | ||
| * @returns {*} The value at the path, or undefined if not found | ||
| */ | ||
| }, { | ||
| key: "getNestedValue", | ||
| value: function getNestedValue(obj, path) { | ||
| if (!obj || _typeof(obj) !== 'object' || !path) { | ||
| return undefined; | ||
| } | ||
|
|
||
| // Handle simple property access (no dots) | ||
| if (path.indexOf('.') === -1) { | ||
| return obj[path]; | ||
| } | ||
|
|
||
| // Handle nested property access | ||
| var keys = path.split('.'); | ||
| var current = obj; | ||
| for (var i = 0; i < keys.length; i++) { | ||
| if (current === null || current === undefined || _typeof(current) !== 'object') { | ||
| return undefined; | ||
| } | ||
| current = current[keys[i]]; | ||
| } | ||
| return current; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Raw-data parsing: handle primitive items consistently and avoid mixed types
- Returning a primitive when a datum is not an object (
return item;) can create heterogeneous arrays where subsequent code expects{x,y(,z)}objects. - Consider a conservative fallback: when
serie.data[0]isn’t an object, coerce primitives into{ x: index, y: parsedNumber }before marking__apexParsed.
- if (_typeof(firstDataPoint) === 'object' && firstDataPoint !== null && (firstDataPoint.hasOwnProperty('x') || firstDataPoint.hasOwnProperty('y')) || Array.isArray(firstDataPoint)) {
+ if (_typeof(firstDataPoint) === 'object' && firstDataPoint !== null && (firstDataPoint.hasOwnProperty('x') || firstDataPoint.hasOwnProperty('y')) || Array.isArray(firstDataPoint)) {
return serie;
}
+ // If data points are primitives (numbers/strings), coerce to {x, y}
+ if (_typeof(firstDataPoint) !== 'object' || firstDataPoint === null) {
+ var coerced = serie.data.map(function (v, idx) {
+ return { x: idx, y: Utils$1.parseNumber(v) };
+ });
+ return _objectSpread2(_objectSpread2({}, serie), {}, { data: coerced, __apexParsed: true });
+ }Optional: extend getNestedValue to support bracket notation (e.g., arr[0].field) in the future.
📝 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.
| }, { | |
| key: "parseRawDataIfNeeded", | |
| value: function parseRawDataIfNeeded(series) { | |
| var _this2 = this; | |
| var cnf = this.w.config; | |
| var gl = this.w.globals; | |
| var globalParsing = cnf.parsing; | |
| // If data was already parsed, don't parse again | |
| if (gl.dataWasParsed) { | |
| return series; | |
| } | |
| // If no global parsing config and no series-level parsing, return as-is | |
| if (!globalParsing && !series.some(function (s) { | |
| return s.parsing; | |
| })) { | |
| return series; | |
| } | |
| var processedSeries = series.map(function (serie, index) { | |
| var _serie$parsing, _serie$parsing2, _serie$parsing3; | |
| if (!serie.data || !Array.isArray(serie.data) || serie.data.length === 0) { | |
| return serie; | |
| } | |
| // Resolve effective parsing config for this series | |
| var effectiveParsing = { | |
| x: ((_serie$parsing = serie.parsing) === null || _serie$parsing === void 0 ? void 0 : _serie$parsing.x) || (globalParsing === null || globalParsing === void 0 ? void 0 : globalParsing.x), | |
| y: ((_serie$parsing2 = serie.parsing) === null || _serie$parsing2 === void 0 ? void 0 : _serie$parsing2.y) || (globalParsing === null || globalParsing === void 0 ? void 0 : globalParsing.y), | |
| z: ((_serie$parsing3 = serie.parsing) === null || _serie$parsing3 === void 0 ? void 0 : _serie$parsing3.z) || (globalParsing === null || globalParsing === void 0 ? void 0 : globalParsing.z) | |
| }; | |
| // If no effective parsing config, return as-is | |
| if (!effectiveParsing.x && !effectiveParsing.y) { | |
| return serie; | |
| } | |
| // Check if data is already in {x, y} format or 2D array format | |
| var firstDataPoint = serie.data[0]; | |
| if (_typeof(firstDataPoint) === 'object' && firstDataPoint !== null && (firstDataPoint.hasOwnProperty('x') || firstDataPoint.hasOwnProperty('y')) || Array.isArray(firstDataPoint)) { | |
| return serie; | |
| } | |
| // Validate that we have both x and y parsing config | |
| if (!effectiveParsing.x || !effectiveParsing.y || Array.isArray(effectiveParsing.y) && effectiveParsing.y.length === 0) { | |
| console.warn("ApexCharts: Series ".concat(index, " has parsing config but missing x or y field specification")); | |
| return serie; | |
| } | |
| // Transform raw data to {x, y} format | |
| var transformedData = serie.data.map(function (item, itemIndex) { | |
| if (_typeof(item) !== 'object' || item === null) { | |
| console.warn("ApexCharts: Series ".concat(index, ", data point ").concat(itemIndex, " is not an object, skipping parsing")); | |
| return item; | |
| } | |
| var x = _this2.getNestedValue(item, effectiveParsing.x); | |
| var y; | |
| var z = undefined; | |
| if (Array.isArray(effectiveParsing.y)) { | |
| var yValues = effectiveParsing.y.map(function (fieldName) { | |
| return _this2.getNestedValue(item, fieldName); | |
| }); | |
| if (_this2.w.config.chart.type === 'bubble' && yValues.length === 2) { | |
| // For bubble: [y-value, z-value] → y = yValues[0], z = yValues[1] | |
| y = yValues[0]; | |
| } else { | |
| y = yValues; | |
| } | |
| } else { | |
| y = _this2.getNestedValue(item, effectiveParsing.y); | |
| } | |
| // explicit z field for bubble charts | |
| if (effectiveParsing.z) { | |
| z = _this2.getNestedValue(item, effectiveParsing.z); | |
| } | |
| // Warn if fields don't exist | |
| if (x === undefined) { | |
| console.warn("ApexCharts: Series ".concat(index, ", data point ").concat(itemIndex, " missing field '").concat(effectiveParsing.x, "'")); | |
| } | |
| if (y === undefined) { | |
| console.warn("ApexCharts: Series ".concat(index, ", data point ").concat(itemIndex, " missing field '").concat(effectiveParsing.y, "'")); | |
| } | |
| var result = { | |
| x: x, | |
| y: y | |
| }; | |
| if (_this2.w.config.chart.type === 'bubble' && Array.isArray(effectiveParsing.y) && effectiveParsing.y.length === 2) { | |
| var zValue = _this2.getNestedValue(item, effectiveParsing.y[1]); | |
| if (zValue !== undefined) { | |
| result.z = zValue; | |
| } | |
| } | |
| if (z !== undefined) { | |
| result.z = z; | |
| } | |
| return result; | |
| }); | |
| return _objectSpread2(_objectSpread2({}, serie), {}, { | |
| data: transformedData, | |
| __apexParsed: true | |
| }); | |
| }); | |
| // Mark that data was parsed | |
| gl.dataWasParsed = true; | |
| if (!gl.originalSeries) { | |
| gl.originalSeries = Utils$1.clone(series); | |
| } | |
| return processedSeries; | |
| } | |
| /** | |
| * Get nested object value using dot notation path | |
| * @param {Object} obj - The object to search in | |
| * @param {string} path - Dot notation path (e.g., 'user.profile.name') | |
| * @returns {*} The value at the path, or undefined if not found | |
| */ | |
| }, { | |
| key: "getNestedValue", | |
| value: function getNestedValue(obj, path) { | |
| if (!obj || _typeof(obj) !== 'object' || !path) { | |
| return undefined; | |
| } | |
| // Handle simple property access (no dots) | |
| if (path.indexOf('.') === -1) { | |
| return obj[path]; | |
| } | |
| // Handle nested property access | |
| var keys = path.split('.'); | |
| var current = obj; | |
| for (var i = 0; i < keys.length; i++) { | |
| if (current === null || current === undefined || _typeof(current) !== 'object') { | |
| return undefined; | |
| } | |
| current = current[keys[i]]; | |
| } | |
| return current; | |
| } | |
| if ( | |
| _typeof(firstDataPoint) === 'object' && | |
| firstDataPoint !== null && | |
| (firstDataPoint.hasOwnProperty('x') || firstDataPoint.hasOwnProperty('y')) || | |
| Array.isArray(firstDataPoint) | |
| ) { | |
| return serie; | |
| } | |
| // If data points are primitives (numbers/strings), coerce to { x, y } | |
| if (_typeof(firstDataPoint) !== 'object' || firstDataPoint === null) { | |
| var coerced = serie.data.map(function (v, idx) { | |
| return { x: idx, y: Utils$1.parseNumber(v) }; | |
| }); | |
| return _objectSpread2( | |
| _objectSpread2({}, serie), | |
| {}, | |
| { data: coerced, __apexParsed: true } | |
| ); | |
| } |
| if (gl.isXNumeric) { | ||
| var diff = Math.round(gl.maxX - gl.minX); | ||
| if (diff < 30) { | ||
| ticks = diff - 1; | ||
| // When numeric range is small, show a tick for every integer | ||
| ticks = diff; | ||
| } | ||
| } | ||
| } else { |
There was a problem hiding this comment.
Avoid zero tick count when range is tiny
When diff is 0 or 1, setting ticks = diff can yield 0 ticks. Clamp to at least 1.
- if (diff < 30) {
- // When numeric range is small, show a tick for every integer
- ticks = diff;
- }
+ if (diff < 30) {
+ // When numeric range is small, show a tick for every integer
+ ticks = Math.max(1, diff);
+ }📝 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 (gl.isXNumeric) { | |
| var diff = Math.round(gl.maxX - gl.minX); | |
| if (diff < 30) { | |
| ticks = diff - 1; | |
| // When numeric range is small, show a tick for every integer | |
| ticks = diff; | |
| } | |
| } | |
| } else { | |
| // replace the existing block at lines 19859–19866 with: | |
| if (diff < 30) { | |
| // When numeric range is small, show a tick for every integer | |
| ticks = Math.max(1, diff); | |
| } |
🤖 Prompt for AI Agents
In assets/js/lib/apexcharts.js around lines 19859 to 19866, the code sets ticks
= diff which can be 0 when diff is 0 or 1; update the logic to clamp ticks to a
minimum of 1 (e.g., ticks = Math.max(1, diff) or equivalent) so that tiny ranges
always produce at least one tick and avoid zero tick count.
| public function __unserialize($modules_data) { | ||
| $this->build_modules($modules_data); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use canonical magic method signature for __unserialize
Type the argument as array and add : void return type to match PHP’s signature.
- public function __unserialize($modules_data) {
+ public function __unserialize(array $modules_data): void {📝 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.
| public function __unserialize($modules_data) { | |
| $this->build_modules($modules_data); | |
| } | |
| public function __unserialize(array $modules_data): void { | |
| $this->build_modules($modules_data); | |
| } |
🤖 Prompt for AI Agents
In inc/objects/class-limitations.php around lines 123 to 125, the magic method
__unserialize lacks the canonical PHP signature; change its declaration to
accept an argument typed as array and add a : void return type (i.e. use
function __unserialize(array $modules_data): void) and keep the existing body
that calls $this->build_modules($modules_data).
| }, | ||
| "scripts": { | ||
| "prebuild": "composer install -o --no-dev", | ||
| "prebuild": "npm run makepot &&composer install -o --no-dev", |
There was a problem hiding this comment.
Broken prebuild script: missing space after && prevents composer from running
"npm run makepot &&composer install -o --no-dev" will not execute composer due to the missing space after &&. This will break releases/CI.
Fix:
- "prebuild": "npm run makepot &&composer install -o --no-dev",
+ "prebuild": "npm run makepot && composer install -o --no-dev",📝 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.
| "prebuild": "npm run makepot &&composer install -o --no-dev", | |
| "prebuild": "npm run makepot && composer install -o --no-dev", |
🤖 Prompt for AI Agents
In package.json around line 26, the prebuild script string lacks a space after
the && which prevents the composer command from running; update the script to
insert a space so it reads "npm run makepot && composer install -o --no-dev"
(i.e., add the missing space between && and composer) and save the file so
CI/releases will run composer as intended.
| public static function set_up_before_class() { | ||
| parent::set_up_before_class(); | ||
|
|
||
| // Create a simple payment object for testing | ||
| // We'll use minimal setup to avoid complex dependencies | ||
| self::$payment = new Payment(); | ||
| self::$payment->set_customer_id(1); | ||
| self::$payment->set_membership_id(1); | ||
| self::$payment->set_currency('USD'); | ||
| self::$payment->set_subtotal(100.00); | ||
| self::$payment->set_total(100.00); | ||
| self::$payment->set_status(Payment_Status::COMPLETED); | ||
| self::$payment->set_gateway('manual'); | ||
|
|
||
| // Save the payment and generate a hash | ||
| $saved = self::$payment->save(); | ||
| if (!$saved) { | ||
| // If save fails, just set a fake hash for testing | ||
| self::$payment->set_hash('test_payment_hash_' . uniqid()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid calling non-existent set_hash() and brittle persistence assumptions.
Payment does not expose set_hash(). Depending on save() to generate a hash also couples the test to DB state and other models’ existence. For the tested code path, any non-empty reference is sufficient to reach nonce/payment checks.
Apply this diff to drop the invalid fallback and decouple from persistence:
// Save the payment and generate a hash
- $saved = self::$payment->save();
- if (!$saved) {
- // If save fails, just set a fake hash for testing
- self::$payment->set_hash('test_payment_hash_' . uniqid());
- }
+ self::$payment->save(); // Best-effort; not required for tests belowFollow-up: In tests below, stop relying on get_hash() when you only need a non-empty reference (see next comments).
📝 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.
| public static function set_up_before_class() { | |
| parent::set_up_before_class(); | |
| // Create a simple payment object for testing | |
| // We'll use minimal setup to avoid complex dependencies | |
| self::$payment = new Payment(); | |
| self::$payment->set_customer_id(1); | |
| self::$payment->set_membership_id(1); | |
| self::$payment->set_currency('USD'); | |
| self::$payment->set_subtotal(100.00); | |
| self::$payment->set_total(100.00); | |
| self::$payment->set_status(Payment_Status::COMPLETED); | |
| self::$payment->set_gateway('manual'); | |
| // Save the payment and generate a hash | |
| $saved = self::$payment->save(); | |
| if (!$saved) { | |
| // If save fails, just set a fake hash for testing | |
| self::$payment->set_hash('test_payment_hash_' . uniqid()); | |
| } | |
| } | |
| public static function set_up_before_class() { | |
| parent::set_up_before_class(); | |
| // Create a simple payment object for testing | |
| // We'll use minimal setup to avoid complex dependencies | |
| self::$payment = new Payment(); | |
| self::$payment->set_customer_id(1); | |
| self::$payment->set_membership_id(1); | |
| self::$payment->set_currency('USD'); | |
| self::$payment->set_subtotal(100.00); | |
| self::$payment->set_total(100.00); | |
| self::$payment->set_status(Payment_Status::COMPLETED); | |
| self::$payment->set_gateway('manual'); | |
| // Save the payment and generate a hash | |
| - $saved = self::$payment->save(); | |
| - if (!$saved) { | |
| - // If save fails, just set a fake hash for testing | |
| - self::$payment->set_hash('test_payment_hash_' . uniqid()); | |
| self::$payment->save(); // Best-effort; not required for tests below | |
| } |
🤖 Prompt for AI Agents
In tests/WP_Ultimo/Managers/Payment_Manager_Test.php around lines 17 to 37, the
test calls a non-existent set_hash() and relies on save() to generate a hash;
remove the invalid fallback and stop depending on persistence. Replace that
fallback by ensuring the Payment object has a non-empty external reference using
the Payment object's public API (e.g. set_reference / set_gateway_reference /
set_transaction_id — whichever exists in the model) or by stubbing/mocking
save() to avoid DB coupling, and update downstream assertions to check for a
non-empty reference instead of get_hash().
| public function test_invoice_viewer_with_invalid_nonce(): void { | ||
| $payment_hash = self::$payment->get_hash(); | ||
| $invalid_nonce = 'invalid_nonce'; | ||
|
|
||
| // Mock the request parameters | ||
| $_REQUEST['action'] = 'invoice'; | ||
| $_REQUEST['reference'] = $payment_hash; | ||
| $_REQUEST['key'] = $invalid_nonce; | ||
|
|
||
| $reflection = new \ReflectionClass($this->payment_manager); | ||
| $method = $reflection->getMethod('invoice_viewer'); | ||
| $method->setAccessible(true); | ||
|
|
||
| // Expect wp_die to be called with permission error | ||
| $this->expectException(\WPDieException::class); | ||
| $this->expectExceptionMessage('You do not have permissions to access this file.'); | ||
|
|
||
| $method->invoke($this->payment_manager); | ||
|
|
||
| // Clean up request parameters | ||
| unset($_REQUEST['action'], $_REQUEST['reference'], $_REQUEST['key']); | ||
| } | ||
|
|
There was a problem hiding this comment.
Ensure nonce path is exercised: don’t use get_hash(); any non-empty reference works.
If get_hash() returns false (unsaved payment), the method will return early before nonce validation and your expectation won’t be met.
Apply this diff:
- $payment_hash = self::$payment->get_hash();
+ $payment_hash = 'dummy_reference_for_invalid_nonce';
@@
- $reflection = new \ReflectionClass($this->payment_manager);
- $method = $reflection->getMethod('invoice_viewer');
- $method->setAccessible(true);
+ // Call directly; method is public.
@@
- $method->invoke($this->payment_manager);
+ $this->payment_manager->invoice_viewer();📝 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.
| public function test_invoice_viewer_with_invalid_nonce(): void { | |
| $payment_hash = self::$payment->get_hash(); | |
| $invalid_nonce = 'invalid_nonce'; | |
| // Mock the request parameters | |
| $_REQUEST['action'] = 'invoice'; | |
| $_REQUEST['reference'] = $payment_hash; | |
| $_REQUEST['key'] = $invalid_nonce; | |
| $reflection = new \ReflectionClass($this->payment_manager); | |
| $method = $reflection->getMethod('invoice_viewer'); | |
| $method->setAccessible(true); | |
| // Expect wp_die to be called with permission error | |
| $this->expectException(\WPDieException::class); | |
| $this->expectExceptionMessage('You do not have permissions to access this file.'); | |
| $method->invoke($this->payment_manager); | |
| // Clean up request parameters | |
| unset($_REQUEST['action'], $_REQUEST['reference'], $_REQUEST['key']); | |
| } | |
| public function test_invoice_viewer_with_invalid_nonce(): void { | |
| $payment_hash = 'dummy_reference_for_invalid_nonce'; | |
| $invalid_nonce = 'invalid_nonce'; | |
| // Mock the request parameters | |
| $_REQUEST['action'] = 'invoice'; | |
| $_REQUEST['reference'] = $payment_hash; | |
| $_REQUEST['key'] = $invalid_nonce; | |
| // Call directly; method is public. | |
| // Expect wp_die to be called with permission error | |
| $this->expectException(\WPDieException::class); | |
| $this->expectExceptionMessage('You do not have permissions to access this file.'); | |
| $this->payment_manager->invoice_viewer(); | |
| // Clean up request parameters | |
| unset($_REQUEST['action'], $_REQUEST['reference'], $_REQUEST['key']); | |
| } |
🤖 Prompt for AI Agents
In tests/WP_Ultimo/Managers/Payment_Manager_Test.php around lines 77 to 99, the
test uses self::$payment->get_hash() which can be false for an unsaved payment
and cause the method to return before nonce validation; replace the call to
get_hash() with any non-empty string (e.g., 'test_reference' or
'non_empty_reference') so the invoice_viewer path that checks the nonce is
exercised, keep the rest of the request setup and cleanup the same.
| public static function tear_down_after_class() { | ||
| global $wpdb; | ||
|
|
||
| // Clean up test data | ||
| if (self::$payment) { | ||
| self::$payment->delete(); | ||
| } | ||
| if (self::$customer) { | ||
| self::$customer->delete(); | ||
| } | ||
|
|
||
| // Clean up database tables | ||
| $wpdb->query("TRUNCATE TABLE {$wpdb->prefix}wu_payments"); | ||
| $wpdb->query("TRUNCATE TABLE {$wpdb->prefix}wu_customers"); | ||
|
|
||
| parent::tear_down_after_class(); | ||
| } |
There was a problem hiding this comment.
Don’t TRUNCATE shared tables in tests.
Truncating wu_payments and wu_customers affects global test state and can break parallel/other suites. Limit cleanup to objects you created, or at least use base_prefix if you truly must operate network-wide (not recommended).
Apply this diff to remove truncation:
- // Clean up database tables
- $wpdb->query("TRUNCATE TABLE {$wpdb->prefix}wu_payments");
- $wpdb->query("TRUNCATE TABLE {$wpdb->prefix}wu_customers");Optionally, to be extra safe, also clear any test-set superglobals between tests:
protected function tear_down(): void {
$_REQUEST = [];
parent::tear_down();
}🤖 Prompt for AI Agents
In tests/WP_Ultimo/Managers/Payment_Manager_Test.php around lines 195-211,
remove the two $wpdb->query("TRUNCATE TABLE ...") calls because truncating
shared tables breaks global/parallel test state; instead rely on deleting the
created self::$payment and self::$customer (already present) and ensure they are
properly unset after deletion; additionally add a protected tear_down(): void
method to clear test-modified superglobals (e.g. $_REQUEST = []) before calling
parent::tear_down(); do not perform network-wide operations like using
base_prefix except under explicit, isolated integration tests.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
inc/class-sunrise.php (2)
438-438: $now should be an integer timestamp (gmdate('U') returns string)Store a real int to keep types consistent and make the system_info formatting work without special casing.
Apply this diff:
- $now = gmdate('U'); + $now = time();
443-447: Unify defaults for last_ to integers; avoid string sentinels*If you adopt numeric defaults in read_sunrise_meta(), align tap() defaults too. This keeps types stable and avoids repeated string comparisons.
$to_save = wp_parse_args( $existing, [ 'active' => false, - 'created' => $now, - 'last_activated' => 'unknown', - 'last_deactivated' => 'unknown', + 'created' => $now, + 'last_activated' => 0, + 'last_deactivated' => 0, ] );
♻️ Duplicate comments (1)
inc/class-sunrise.php (1)
311-324: Stop using 'unknown' string sentinels in sunrise_meta defaults; store integers insteadUsing strings for timestamps forces type checks everywhere and risks regressions wherever formatting is applied. Prefer numeric epoch defaults (0) for created/last_* to keep types consistent and eliminate surprising behavior.
Apply this diff:
- self::$sunrise_meta = get_network_option( + self::$sunrise_meta = get_network_option( null, 'wu_sunrise_meta', [ 'active' => false, - 'created' => 'unknown', - 'last_activated' => 'unknown', - 'last_deactivated' => 'unknown', - 'last_modified' => 'unknown', + 'created' => 0, + 'last_activated' => 0, + 'last_deactivated' => 0, + 'last_modified' => 0, ] );
🧹 Nitpick comments (2)
inc/class-sunrise.php (2)
450-452: Normalize legacy values robustly (not just 'unknown')Sites may already have stringified timestamps or empty values. Normalize to an integer if the value is non-numeric or <= 0.
- if ('unknown' === $to_save['created']) { - $to_save['created'] = $now; - } + if (!is_numeric($to_save['created']) || (int) $to_save['created'] <= 0) { + $to_save['created'] = $now; + }
56-59: Duplicate include of functions/debug.phpdebug.php is required twice; remove the redundant include.
require_once __DIR__ . '/functions/debug.php'; - - require_once __DIR__ . '/functions/debug.php';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
inc/class-sunrise.php(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: cypress (8.0, firefox)
- GitHub Check: cypress (8.4, firefox)
- GitHub Check: cypress (8.0, chrome)
- GitHub Check: cypress (8.2, chrome)
- GitHub Check: cypress (8.4, chrome)
- GitHub Check: cypress (8.2, firefox)
- GitHub Check: cypress (8.3, chrome)
- GitHub Check: cypress (8.3, firefox)
- GitHub Check: cypress (7.4, firefox)
- GitHub Check: cypress (8.1, chrome)
- GitHub Check: cypress (8.1, firefox)
- GitHub Check: cypress (7.4, chrome)
🔇 Additional comments (2)
inc/class-sunrise.php (2)
118-121: Interfaces loaded early — looks correctThe new interface requires are in the right place (before traits/classes that might implement them). No issues spotted.
311-324: No other ‘unknown’ string comparisons in sunrise_meta logicI’ve verified that aside from the existing created‐default check in tap(), there are no other parts of the codebase comparing any sunrise_meta timestamp fields against the string “unknown.” Display routines in inc/class-sunrise.php simply use
is_int(…)checks to choose between formatting an integer timestamp or echoing the string value, so unifying timestamp types won’t break additional logic.
| 'sunrise-created' => [ | ||
| 'tooltip' => '', | ||
| 'title' => 'Created', | ||
| 'value' => gmdate('Y-m-d @ H:i:s', $data['created']), | ||
| 'value' => is_int($data['created']) ? gmdate('Y-m-d @ H:i:s', $data['created']) : $data['created'], | ||
| ], | ||
| 'sunrise-last-activated' => [ | ||
| 'tooltip' => '', | ||
| 'title' => 'Last Activated', | ||
| 'value' => gmdate('Y-m-d @ H:i:s', $data['last_activated']), | ||
| 'value' => is_int($data['last_activated']) ? gmdate('Y-m-d @ H:i:s', $data['last_activated']) : $data['last_activated'], | ||
| ], | ||
| 'sunrise-last-deactivated' => [ | ||
| 'tooltip' => '', | ||
| 'title' => 'Last Deactivated', | ||
| 'value' => gmdate('Y-m-d @ H:i:s', $data['last_deactivated']), | ||
| 'value' => is_int($data['last_deactivated']) ? gmdate('Y-m-d @ H:i:s', $data['last_deactivated']) : $data['last_deactivated'], | ||
| ], | ||
| 'sunrise-last-modified' => [ | ||
| 'tooltip' => '', | ||
| 'title' => 'Last Modified', | ||
| 'value' => gmdate('Y-m-d @ H:i:s', $data['last_modified']), | ||
| 'value' => is_int($data['last_modified']) ? gmdate('Y-m-d @ H:i:s', $data['last_modified']) : $data['last_modified'], | ||
| ], |
There was a problem hiding this comment.
is_int() guard won’t ever format: values are stored as strings elsewhere
Because tap() sets timestamps via gmdate('U') (a string), these fields won’t pass is_int(), so system_info will output raw numbers instead of formatted dates. Either store ints or accept numeric strings in the formatter.
Two compatible options (doing both is safest during migration):
- Option A (formatter hardening): accept numeric strings and cast
- 'value' => is_int($data['created']) ? gmdate('Y-m-d @ H:i:s', $data['created']) : $data['created'],
+ 'value' => is_numeric($data['created']) ? gmdate('Y-m-d @ H:i:s', (int) $data['created']) : $data['created'],
@@
- 'value' => is_int($data['last_activated']) ? gmdate('Y-m-d @ H:i:s', $data['last_activated']) : $data['last_activated'],
+ 'value' => is_numeric($data['last_activated']) ? gmdate('Y-m-d @ H:i:s', (int) $data['last_activated']) : $data['last_activated'],
@@
- 'value' => is_int($data['last_deactivated']) ? gmdate('Y-m-d @ H:i:s', $data['last_deactivated']) : $data['last_deactivated'],
+ 'value' => is_numeric($data['last_deactivated']) ? gmdate('Y-m-d @ H:i:s', (int) $data['last_deactivated']) : $data['last_deactivated'],
@@
- 'value' => is_int($data['last_modified']) ? gmdate('Y-m-d @ H:i:s', $data['last_modified']) : $data['last_modified'],
+ 'value' => is_numeric($data['last_modified']) ? gmdate('Y-m-d @ H:i:s', (int) $data['last_modified']) : $data['last_modified'],- Option B (preferred): store ints at the source; see the tap() comments below to switch from string to int.
📝 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.
| 'sunrise-created' => [ | |
| 'tooltip' => '', | |
| 'title' => 'Created', | |
| 'value' => gmdate('Y-m-d @ H:i:s', $data['created']), | |
| 'value' => is_int($data['created']) ? gmdate('Y-m-d @ H:i:s', $data['created']) : $data['created'], | |
| ], | |
| 'sunrise-last-activated' => [ | |
| 'tooltip' => '', | |
| 'title' => 'Last Activated', | |
| 'value' => gmdate('Y-m-d @ H:i:s', $data['last_activated']), | |
| 'value' => is_int($data['last_activated']) ? gmdate('Y-m-d @ H:i:s', $data['last_activated']) : $data['last_activated'], | |
| ], | |
| 'sunrise-last-deactivated' => [ | |
| 'tooltip' => '', | |
| 'title' => 'Last Deactivated', | |
| 'value' => gmdate('Y-m-d @ H:i:s', $data['last_deactivated']), | |
| 'value' => is_int($data['last_deactivated']) ? gmdate('Y-m-d @ H:i:s', $data['last_deactivated']) : $data['last_deactivated'], | |
| ], | |
| 'sunrise-last-modified' => [ | |
| 'tooltip' => '', | |
| 'title' => 'Last Modified', | |
| 'value' => gmdate('Y-m-d @ H:i:s', $data['last_modified']), | |
| 'value' => is_int($data['last_modified']) ? gmdate('Y-m-d @ H:i:s', $data['last_modified']) : $data['last_modified'], | |
| ], | |
| // In inc/class-sunrise.php, inside the system_info() array construction: | |
| 'sunrise-created' => [ | |
| 'tooltip' => '', | |
| 'title' => 'Created', | |
| 'value' => is_numeric($data['created']) | |
| ? gmdate('Y-m-d @ H:i:s', (int) $data['created']) | |
| : $data['created'], | |
| ], | |
| 'sunrise-last-activated' => [ | |
| 'tooltip' => '', | |
| 'title' => 'Last Activated', | |
| 'value' => is_numeric($data['last_activated']) | |
| ? gmdate('Y-m-d @ H:i:s', (int) $data['last_activated']) | |
| : $data['last_activated'], | |
| ], | |
| 'sunrise-last-deactivated' => [ | |
| 'tooltip' => '', | |
| 'title' => 'Last Deactivated', | |
| 'value' => is_numeric($data['last_deactivated']) | |
| ? gmdate('Y-m-d @ H:i:s', (int) $data['last_deactivated']) | |
| : $data['last_deactivated'], | |
| ], | |
| 'sunrise-last-modified' => [ | |
| 'tooltip' => '', | |
| 'title' => 'Last Modified', | |
| 'value' => is_numeric($data['last_modified']) | |
| ? gmdate('Y-m-d @ H:i:s', (int) $data['last_modified']) | |
| : $data['last_modified'], | |
| ], |
🤖 Prompt for AI Agents
inc/class-sunrise.php around lines 351 to 370: the current is_int() guards never
match because timestamps are stored as numeric strings (gmdate('U')), so dates
are not formatted; fix by either (A) making the formatter accept numeric strings
(use is_numeric() or cast to int before passing to gmdate) so string timestamps
are formatted, and/or (B — preferred) change the source that writes timestamps
to store integers (use (int) time() or intval(gmdate('U'))); doing both
(hardening the formatter and migrating stored values to ints) is safest during
rollout.
Adds some tests and a few key hooks for use by addons
Summary by CodeRabbit
New Features
Bug Fixes
Tests