Skip to content

fix: resolve SMTP Configuration link 404 on API fallback#17

Open
Rat01047 wants to merge 2 commits intodevelopfrom
fix/smtp-configuration-redirect-url
Open

fix: resolve SMTP Configuration link 404 on API fallback#17
Rat01047 wants to merge 2 commits intodevelopfrom
fix/smtp-configuration-redirect-url

Conversation

@Rat01047
Copy link

@Rat01047 Rat01047 commented Mar 13, 2026

Issue-Description:

The SMTP Configuration button on the Email tab redirected to https://app.flywp.com/email (404) instead of https://app.flywp.com/site/{site_id}/email when site_info was unavailable (API failure / expired transient).

Fix-of-Issue:

In includes/Admin.php only:

  • Persist site_id as a WordPress option when the API returns it successfully
  • Use the stored option as fallback in get_site_url() when $site_info is false

Impact:

  • No view changes, no unrelated logic touched
  • Safe fallback to https://app.flywp.com if site_id has never been stored

Summary by CodeRabbit

  • Bug Fixes
    • Improved site URL generation and storage to ensure consistent site identification and reliability

@coderabbitai
Copy link

coderabbitai bot commented Mar 13, 2026

Walkthrough

The 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

Cohort / File(s) Summary
Site ID Caching & URL Derivation
includes/Admin.php
Modified site_info caching to persist site ID to flywp_site_id option; updated get_site_url to derive site_id from info['id'] or stored option, with fallback to base app URL for site-specific URL construction.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 Site IDs now dance in the cache,
Persisted with care, a swift dash,
From info to storage they flow,
URLs built with what we now know,
No site left without its true glow! 🌟

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title describes resolving a SMTP Configuration link 404 issue on API fallback, which matches the PR's main objective of fixing the email redirect URL when site_info is unavailable.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/smtp-configuration-redirect-url
📝 Coding Plan
  • Generate coding plan for human review comments

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Tip

CodeRabbit can suggest fixes for GitHub Check annotations.

Configure the reviews.tools.github-checks setting to adjust the time to wait for GitHub Checks to complete.

@Rat01047 Rat01047 requested a review from itsrafsanjani March 13, 2026 09:49
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
includes/Admin.php (1)

132-135: Add lifecycle cleanup for persisted flywp_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.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d9443962-bbb9-419f-908f-284b16363802

📥 Commits

Reviewing files that changed from the base of the PR and between 644fe87 and dccca48.

📒 Files selected for processing (1)
  • includes/Admin.php

Copy link
Member

@itsrafsanjani itsrafsanjani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants