Legacy license field improvements - #2960
Conversation
1. If unified license key is set, the legacy license fields are made readonly and disabled. 2. Warn user away from entering unified license key to the legacy license fields. Guide to the LW license manager. 3. Fix bug where incorrect message is show when user tried to enter legacy license when already unified license key is set.
| return $this->is_product_licensed( | ||
| $this->get_harbor_product_slug( $tec_product_slug ) | ||
| ); |
There was a problem hiding this comment.
This specific check could be risky, right? It just checks whether their Unified Key covers the Product Slug. Not whether it is active for their site.
If they choose specifically not to activate the Product License on their site via their Unified Key, then they should be able to enter a Legacy Key still.
lw_harbor_is_product_license_active() may be the correct underlying function to call here. $this->is_product_license() calls lw_harbor_is_feature_available() under the hood which is a different check.
There was a problem hiding this comment.
@d4mation I have made this change 07c8007 (this PR)
Questions:
- Are entitlements same as capabilities? Do we need to to rename the function?
- Do you think this function could be helpful for other plugins as well and can be added to Harbor's global functions?
There was a problem hiding this comment.
Ah, good catch on this! I was working on LearnDash Core recently where I used lw_harbor_is_product_license_active() and it just happened to be the same slug as the Product in Portal+Licensing.
In this case, you need to be checking for Capabilities (plugins, services, etc.) that are provided via the Product. What you have done looks correct from a logic standpoint, but it will need to be a global function in Harbor to avoid calling the version frozen within Tribe Common. https://github.com/stellarwp/harbor/blob/main/docs/architecture/fat-leader-thin-instance.md
It might be a good idea to expose a few functions here:
- One to get all the Products from the License Repository
- One that does what you're doing and checking if a given Capability Slug is covered by the entered Unified Key, whether or not the Product is active for the domain
- One that does the same as above, but also checks whether the Product is active for the domain (exactly what you're doing)
There was a problem hiding this comment.
@d4mation Sure, I'm on it. Do these names make sense to you:
lw_harbor_get_licensed_products()
lw_harbor_is_capability_licensed( string $capability_slug ): bool
function lw_harbor_is_capability_license_active( string $capability_slug ): bool
There was a problem hiding this comment.
I think we should follow some existing naming patterns:
lw_harbor_is_capability_license_available( string $slug ): boollw_harbor_is_capability_license_active( string $slug ): bool
But I otherwise prefer your simplification here :)
I am less sure about lw_harbor_get_licensed_products() though. Unless you think you need it, we should skip it for now. We would need to flatten it to an array of string slugs to make it make sense in a multi-Harbor environment because each version of Harbor would be expecting a different Strauss Prefix on whatever Object it would normally want to return.
…bilities `is_license_field_managed_by_harbor()` was checking tier entitlement (`is_product_licensed()`), which only confirms a customer's tier covers a product, not that it's actually licensed for this site. It also didn't account for accounts holding license entries for multiple sites, so a capability granted only to another site's entry could incorrectly read as active here. Add `is_capability_license_active()`, which checks the `capabilities` list of `Product_Entry` rows that are both valid and `activated_here`, matching the real shape of Harbor's licensing data (one entry per bundle+tier, add-ons listed as capabilities rather than their own product slug).
…ve instead of creating a new one
| "stellarwp/container-contract": "^1.0.4", | ||
| "stellarwp/db": "^1.0.3", | ||
| "stellarwp/harbor": "^1.4", | ||
| "stellarwp/harbor": "dev-SMTNC-1895", |
There was a problem hiding this comment.
Lets make sure we release new version of harbor before merging so we dont ship a dev version.
There was a problem hiding this comment.
This has been reverted back to ^1.4 since we dont need any new functions, we are now utilising the existing functions lw_harbor_is_feature_enabled 👍
johnhooks
left a comment
johnhooks
left a comment
There was a problem hiding this comment.
@pramodjodhani I approve. From my perspective this handles the juggling of unified and stellar license key correctly.
Though please get full approval from @dpanta94 before merging, he knows the ins and outs of tribe-common, I do not.
2. Restore submodule reference to src/resources/postcss/utilities 3. Added type hints to the PUE functions
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughHarbor now detects unified license keys, manages legacy license fields, intercepts PUE validation and saving, and provides localized error messaging. The PUE checker supports pre-validation short-circuiting. Integration tests cover managed and unmanaged product behavior. ChangesHarbor license handling
Sequence Diagram(s)sequenceDiagram
participant Admin
participant PUEChecker
participant Harbor
participant Options
Admin->>PUEChecker: Submit a license key
PUEChecker->>Harbor: Apply pre-validation filter
Harbor-->>PUEChecker: Return a response or continue validation
Harbor->>Options: Preserve or reject the submitted value
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Common/Libraries/Harbor.php`:
- Around line 308-315: Update the legacy PUE methods filter_pre_get_option(),
filter_stellarwp_uplink_tec_license_get_key(), filter_pue_get_update_url(), and
filter_pre_http_request() to use the active-capability predicate,
is_license_field_managed_by_harbor(), instead of is_product_licensed(). Preserve
existing behavior for activated capabilities and add coverage for an entitled
but inactive capability.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a5032be1-c2d5-4431-8e57-dcceccc5cfdf
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
changelog/update-license-field-improvementscomposer.jsonsrc/Common/Integrations/Harbor/PUE.phpsrc/Common/Libraries/Harbor.phpsrc/Tribe/PUE/Checker.phptests/integration/Tribe/Common/Integrations/Harbor/PUE_Test.phptests/integration/Tribe/Common/Libraries/Harbor_Test.php
| public function is_license_field_managed_by_harbor( string $tec_product_slug ): bool { | ||
| if ( ! lw_harbor_has_unified_license_key() ) { | ||
| return false; | ||
| } | ||
|
|
||
| return lw_harbor_is_capability_license_active( | ||
| $this->get_harbor_product_slug( $tec_product_slug ) | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Use the active-capability check for all legacy PUE paths.
Line 313 correctly checks whether the capability is active on this site. However, PUE::filter_pre_get_option(), PUE::filter_stellarwp_uplink_tec_license_get_key(), PUE::filter_pue_get_update_url(), and PUE::filter_pre_http_request() still use is_product_licensed(), which checks feature availability.
If a customer is entitled to a capability but has not activated it on this site, the field remains editable but PUE still reads and uses the unified key. A saved legacy key cannot become effective.
Use the active-capability predicate for these legacy PUE paths. Add coverage with an entitled capability that is not activated on the current site.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Common/Libraries/Harbor.php` around lines 308 - 315, Update the legacy
PUE methods filter_pre_get_option(),
filter_stellarwp_uplink_tec_license_get_key(), filter_pue_get_update_url(), and
filter_pre_http_request() to use the active-capability predicate,
is_license_field_managed_by_harbor(), instead of is_product_licensed(). Preserve
existing behavior for activated capabilities and add coverage for an entitled
but inactive capability.
There was a problem hiding this comment.
@dpanta94 I think this comment makes sense and should be implemented. But I want to confirm this with you, as its possible I might have not full context of why is_product_licensed was used in these places.
is_product_licensed internally calls lw_harbor_is_feature_available which only checks if the feature is available in the tier, not whether its active or not.
…d - update the error message to guide user accordingly
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/integration/Tribe/Common/Libraries/Harbor_Test.php (1)
208-214: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUpdate the expected manager label.
Line 213 expects
LW License Manager, butHarbor::get_unified_license_key_entry_error_message()emitsUnified License Manager. This test fails.Proposed fix
- $this->assertStringContainsString( 'LW License Manager', $message ); + $this->assertStringContainsString( 'Unified License Manager', $message );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/Tribe/Common/Libraries/Harbor_Test.php` around lines 208 - 214, Update the expected manager label in it_should_include_license_manager_link_in_unified_key_entry_error_message to match the Unified License Manager text emitted by Harbor::get_unified_license_key_entry_error_message, while preserving the existing link and unified-license assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/integration/Tribe/Common/Libraries/Harbor_Test.php`:
- Around line 208-214: Update the expected manager label in
it_should_include_license_manager_link_in_unified_key_entry_error_message to
match the Unified License Manager text emitted by
Harbor::get_unified_license_key_entry_error_message, while preserving the
existing link and unified-license assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4453db09-ef06-47ce-a6c5-e1c7731fdd40
📒 Files selected for processing (5)
src/Common/Integrations/Harbor/PUE.phpsrc/Common/Libraries/Harbor.phpsrc/Tribe/PUE/Checker.phptests/integration/Tribe/Common/Integrations/Harbor/PUE_Test.phptests/integration/Tribe/Common/Libraries/Harbor_Test.php
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Tribe/PUE/Checker.php
- tests/integration/Tribe/Common/Integrations/Harbor/PUE_Test.php
- src/Common/Integrations/Harbor/PUE.php
…w_harbor_is_feature_enabled instead of creating a new one called we created in harbod (lw_harbor_is_capability_license_active)
- spy that remote validation is skipped - cover pre_validate paths via data provider
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/integration/Tribe/Common/Libraries/Harbor_Test.php (3)
130-130: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winExercise the registered PUE filter in the pass-through cases.
Line 130 and Line 158 call Harbor's callback directly. These tests do not verify that
tec_common_pue_pre_validate_keyis registered or has the correct priority. Useapply_filters()as in Line 100 for these integration tests. Keep direct callback calls in separate unit tests.Also applies to: 158-158
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/Tribe/Common/Libraries/Harbor_Test.php` at line 130, Update the pass-through integration tests around Harbor::filter_tec_common_pue_pre_validate_key, including the cases at lines 130 and 158, to invoke the registered tec_common_pue_pre_validate_key filter via apply_filters() with the same arguments. Preserve direct callback invocation only in separate unit tests.
100-111: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the License Manager link destination.
Line 100 through Line 111 checks the label, and Line 212 checks only that an anchor exists. A wrong or stale
hrefwould still pass. Assert that both messages contain the configured Unified License Manager URL, such astribe( Harbor::class )->get_portal_url().Also applies to: 208-213
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/Tribe/Common/Libraries/Harbor_Test.php` around lines 100 - 111, Update the assertions in the test cases around the unified license key message, including the block using apply_filters and the anchor check near the second occurrence, to verify each message contains the configured Unified License Manager URL from tribe( Harbor::class )->get_portal_url(). Preserve the existing label and anchor assertions while adding the destination assertion.
90-104: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRestore the Harbor action state in
finallyblocks.The test restores
$wp_actions['lw_harbor/loaded']only after the callback returns. Ifapply_filters()or a callback throws, later tests can inherit modified global state. The first two blocks also do not explicitly unset the key when it was absent before the test. Wrap each setup and filter call intry/finally, and restore both the prior value and key presence.Also applies to: 120-134, 148-164
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/Tribe/Common/Libraries/Harbor_Test.php` around lines 90 - 104, Update each affected test block around apply_filters(), including the blocks near the visible Tribe__PUE__Checker setup and the additional referenced blocks, to wrap setup, invocation, and restoration in try/finally. Preserve whether $wp_actions['lw_harbor/loaded'] originally existed: restore its saved value when present, otherwise explicitly unset the key in the finally block, even when the filter or callback throws.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/integration/Tribe/Common/Libraries/Harbor_Test.php`:
- Line 130: Update the pass-through integration tests around
Harbor::filter_tec_common_pue_pre_validate_key, including the cases at lines 130
and 158, to invoke the registered tec_common_pue_pre_validate_key filter via
apply_filters() with the same arguments. Preserve direct callback invocation
only in separate unit tests.
- Around line 100-111: Update the assertions in the test cases around the
unified license key message, including the block using apply_filters and the
anchor check near the second occurrence, to verify each message contains the
configured Unified License Manager URL from tribe( Harbor::class
)->get_portal_url(). Preserve the existing label and anchor assertions while
adding the destination assertion.
- Around line 90-104: Update each affected test block around apply_filters(),
including the blocks near the visible Tribe__PUE__Checker setup and the
additional referenced blocks, to wrap setup, invocation, and restoration in
try/finally. Preserve whether $wp_actions['lw_harbor/loaded'] originally
existed: restore its saved value when present, otherwise explicitly unset the
key in the finally block, even when the filter or callback throws.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 834aee82-c5c2-4097-80ca-679c24acc3c1
📒 Files selected for processing (3)
src/Common/Libraries/Harbor.phptests/integration/Tribe/Common/Integrations/Harbor/PUE_Test.phptests/integration/Tribe/Common/Libraries/Harbor_Test.php
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/integration/Tribe/Common/Integrations/Harbor/PUE_Test.php
- src/Common/Libraries/Harbor.php
|
@dpanta94 This is ready for review again. There is a test failing, I checked it and found that the test script is crashing because the control goes to the exit statement here: https://github.com/the-events-calendar/the-events-calendar/blob/31784e78fbb240feca0cbfb506e7426de9c594b9/src/Events/Admin/Onboarding/Controller.php#L237 I confirmed this by commenting out the tribe_exit() function and then the test script was working fine except some failures unrelated to this PR (It_should_return_the_current_theme_object, It_should_correctly_identify_an_active_theme) How do you think we should approach this solution? Maybe we should stub |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/wpunit/_bootstrap.php`:
- Line 14: Remove the suite-wide `tribe_exit` filter from the bootstrap. In the
specific test that needs to intercept `tribe_exit()`, scope the filter to that
test or replace it with an exception-based handler that stops execution at the
exit boundary, while preserving normal termination behavior for all other tests.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: be3f1bb0-94f7-4c53-9d0e-2b43badfca5f
📒 Files selected for processing (3)
src/Common/Libraries/Harbor.phptests/integration/Tribe/Common/Integrations/Harbor/PUE_Test.phptests/wpunit/_bootstrap.php
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/integration/Tribe/Common/Integrations/Harbor/PUE_Test.php
- src/Common/Libraries/Harbor.php
| * function in src/Events/Admin/Onboarding/Controller.php) could kill the PHP process and | ||
| * silently abandon every remaining test in the suite. | ||
| */ | ||
| tests_add_filter( 'tribe_exit', static fn() => '__return_true' ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Review all exit-related tests and filters.
rg -n -C 4 'tribe_exit|tests_add_filter|remove_filter' tests srcRepository: the-events-calendar/tribe-common
Length of output: 44020
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- tribe_exit call sites in WPUnit tests ---'
rg -n -C 5 'tribe_exit|maybe_redirect_to_guided_setup_on_activation|Onboarding' tests/wpunit src/Events tests/_bootstrap.php tests/wpunit/_bootstrap.php 2>/dev/null || true
printf '%s\n' '--- bootstrap and exit wrapper ---'
cat -n tests/wpunit/_bootstrap.php
sed -n '120,150p' src/functions/utils.php
printf '%s\n' '--- test bootstrap/filter helpers ---'
rg -n -C 5 'function tests_add_filter|tests_add_filter|setUpBeforeClass|tearDownAfterClass' tests src --glob '*.php'Repository: the-events-calendar/tribe-common
Length of output: 21303
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- WPUnit configuration and process isolation ---'
fd -i 'codeception|phpunit|bootstrap|config' . -t f | head -200
rg -n -C 4 'wpunit|processIsolation|process-isolation|bootstrap|tests_add_filter' . \
--glob '!vendor/**' --glob '!node_modules/**' --glob '!*.lock' | head -400
printf '%s\n' '--- All tracked tribe_exit call sites ---'
rg -n -C 3 'tribe_exit\s*\(' . \
--glob '*.php' --glob '!vendor/**' --glob '!node_modules/**'
printf '%s\n' '--- Onboarding-related tracked files ---'
fd -i 'onboard|guided|activation' . -t fRepository: the-events-calendar/tribe-common
Length of output: 36430
Avoid a suite-wide tribe_exit() no-op.
This filter applies to the complete WPUnit process. Every tribe_exit() call invokes __return_true instead of terminating. Code after redirects or other exit paths can therefore run and hide regressions. Scope the filter to the test that requires it, or use an exception-based handler that preserves the exit boundary.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/wpunit/_bootstrap.php` at line 14, Remove the suite-wide `tribe_exit`
filter from the bootstrap. In the specific test that needs to intercept
`tribe_exit()`, scope the filter to that test or replace it with an
exception-based handler that stops execution at the exit boundary, while
preserving normal termination behavior for all other tests.
|
@dpanta94 because |
|
@pramodjodhani yes i do think otherwise. In tests environment we need to have control, mocking fully a function may mean that in the future we lose something we should catch. Do you want to take a look here ? Im suspecting cherry picking that commit would fix the suite. |
|
@dpanta94 Fair enough. I have added that commit and removed my code from bootstrap.php. Everything seems to be in good shape now. Can you please confirm? |
4 participants
🎫 Ticket
SMTNC-1862
🗒️ Description
🎥 Artifacts
https://www.loom.com/share/94f08029f9fc4ac0a4a1f08915e1e778
✔️ Checklist
npm run changelogto add changelog file(s). More info herewpunitorintegrationtests.wpunitorintegrationtests.