fix: resolve SMTP Configuration link 404 on API fallback#17
Conversation
WalkthroughThe code now persists the site ID to the flywp_site_id option after caching site_info, and get_site_url derives the site ID from cached info or the stored option with fallback to the base app URL. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
includes/Admin.php (1)
132-135: Add lifecycle cleanup for persistedflywp_site_id.Line 134 persists
flywp_site_id, but there’s no matching cleanup path (e.g., deactivation/uninstall). That can leave stale IDs in cloned/restored environments and cause incorrect fallback redirects during API/transient failures.Possible follow-up (outside this file)
public function deactivate() { $timestamp = wp_next_scheduled( FlyWP\Api\UpdatesData::CRON_HOOK ); if ( $timestamp ) { wp_unschedule_event( $timestamp, FlyWP\Api\UpdatesData::CRON_HOOK ); } + delete_option( 'flywp_site_id' ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@includes/Admin.php` around lines 132 - 135, Persisting flywp_site_id via update_option('flywp_site_id', ...) lacks lifecycle cleanup; add a cleanup routine that deletes the stored option on plugin deactivation and uninstall (e.g., implement a flywp_cleanup_site_id function that calls delete_option('flywp_site_id') and hook it using register_deactivation_hook(...) and either register_uninstall_hook(...) or an uninstall.php that calls flywp_cleanup_site_id) so cloned/restored sites don’t retain stale IDs and cause incorrect redirects.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@includes/Admin.php`:
- Around line 132-135: Persisting flywp_site_id via
update_option('flywp_site_id', ...) lacks lifecycle cleanup; add a cleanup
routine that deletes the stored option on plugin deactivation and uninstall
(e.g., implement a flywp_cleanup_site_id function that calls
delete_option('flywp_site_id') and hook it using register_deactivation_hook(...)
and either register_uninstall_hook(...) or an uninstall.php that calls
flywp_cleanup_site_id) so cloned/restored sites don’t retain stale IDs and cause
incorrect redirects.
|
Fixes: #21 |
|
@Rat01047 bhai, test it using the |
|
@Rat01047 bhai, do something like this for the admin side endpoint too: /**
* Get the API endpoint.
*
* @return string
*/
protected function get_endpoint() {
return apply_filters( 'flywp_api_endpoint', 'https://app.flywp.com/api/site-api' );
} |
Issue-Description:
The SMTP Configuration button on the Email tab redirected to
https://app.flywp.com/email(404) instead ofhttps://app.flywp.com/site/{site_id}/emailwhensite_infowas unavailable (API failure / expired transient).Fix-of-Issue:
In
includes/Admin.phponly:site_idas a WordPress option when the API returns it successfullyget_site_url()when$site_info is falseImpact:
https://app.flywp.comifsite_idhas never been storedSummary by CodeRabbit
Fixes: #21