fix(vcs): honour the console URL scheme in installation callback defaults - #13599
Open
HarshMN2345 wants to merge 3 commits into
Open
fix(vcs): honour the console URL scheme in installation callback defaults#13599HarshMN2345 wants to merge 3 commits into
HarshMN2345 wants to merge 3 commits into
Conversation
…ults
The commit statuses and the authorize-contributor link were gated on
_APP_CONSOLE_URL_SCHEME, but the installation callbacks were not: when
state carries no success or failure URL, all three providers fell back to
/console/project-{region}-{id}/settings/git-installations. On a root-scheme
install that path does not exist, so an install started from the provider's
side landed on a dead page.
Build the fallback from the same flag, pointing root-scheme installs at the
project settings page.
The Gitea suite runs against both schemes -- server-ce sets root, cloud
leaves the default -- so the two assertions on the fallback now check that
it targets the project rather than pinning one scheme's path.
Contributor
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } else { | ||
| // Nothing was signed into state, so the callback falls back to its own | ||
| // console URL, whose shape follows _APP_CONSOLE_URL_SCHEME. | ||
| $this->assertStringContainsString($projectId, $location); |
Contributor
There was a problem hiding this comment.
This assertion only requires the redirect to contain the project ID. The previous broken /console/project-{region}-{projectId}/... URL satisfies that condition even under the root scheme. The failure-path checks at lines 541–542 have the same problem because the old URL also contains ?error=. Reverting the production fix would therefore leave both tests green. Add an observable scheme-specific check, such as validating the redirect in an environment with a known console scheme, without duplicating the production configuration branch in the test.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/e2e/Services/VCSGitea/VCSGiteaConsoleClientTest.php
Line: 322
Comment:
**Fallback regression goes undetected**
This assertion only requires the redirect to contain the project ID. The previous broken `/console/project-{region}-{projectId}/...` URL satisfies that condition even under the root scheme. The failure-path checks at lines 541–542 have the same problem because the old URL also contains `?error=`. Reverting the production fix would therefore leave both tests green. Add an observable scheme-specific check, such as validating the redirect in an environment with a known console scheme, without duplicating the production configuration branch in the test.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
✨ Benchmark resultsComparing
Per-scenario breakdown & investigation detailsMetrics below reflect the current branch (after). Δ P95 compares against the base.
Top API waits (after)
|
The previous commit relaxed these two assertions on the premise that a test cannot see _APP_CONSOLE_URL_SCHEME. That premise was wrong: both CI stacks exec the suite inside the appwrite container, and this file already reads _APP_OPENSSL_KEY_V1, _APP_REGION and _APP_VCS_GITEA_ENDPOINT from that environment. Substring checks on the project id were invariant along the one axis the fix changes -- both URL shapes interpolate the id -- so inverting or deleting the ternary stayed green. Read the flag and assert the exact URL for whichever scheme is running, which also restores the region coverage cloud relies on.
bin/worker-jobs reaches GitAction::run() through Functions/Workers/Jobs.php, so it writes the commit-status target URLs and the PR comment's deployment links. Its service was the one Git-linking container never given _APP_CONSOLE_URL_SCHEME, _APP_CONSOLE_DOMAIN, _APP_DOMAIN or _APP_OPTIONS_FORCE_HTTPS: the flag was added to appwrite, appwrite-worker, appwrite-worker-webhooks and appwrite-worker-builds and this service, which already existed, was skipped. On a --profile separate stack every link it wrote fell back to the legacy console paths, which the root-scheme console does not serve.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Gates the VCS installation callbacks' default redirect on
_APP_CONSOLE_URL_SCHEME, the same flag the commit statuses and the authorize-contributor link already use.All three callbacks — the shared OAuth2 base, GitHub, and Origin — hardcoded their fallback to
/console/project-{region}-{projectId}/settings/git-installations. On a root-scheme install that path does not exist, so the user landed on a dead page. Root scheme now falls back to/projects/{projectId}/settings.The fallback only fires when
statecarries nosuccess/failureURL. The console always signs those in, but an install started from the provider's side (installing the GitHub App straight from GitHub, for example) reaches the callback without them, which is exactly the path that was broken.Cloud is a strict no-op: it sets no
_APP_CONSOLE_URL_SCHEME, so its legacy branch is byte-identical to the string that was there before.Test Plan
tests/e2e/Services/VCSGitea/VCSGiteaConsoleClientTest.phpcovers the fallback in two places:createInstallationHelper(redirects: false)andtestCreateInstallationWithEmptyRedirects.That suite runs under both schemes — server-ce's
docker-compose.ymlsets_APP_CONSOLE_URL_SCHEME=root, cloud leaves it at the default. Both CI stacks exec the suite inside the appwrite container, so the tests share its environment;defaultRedirectUrl()reads the flag and asserts the exact expected URL for whichever scheme is running, the same way this file already reads_APP_OPENSSL_KEY_V1and_APP_REGION.The second commit exists because the first one relaxed those assertions to substring checks on the project id, on the mistaken premise that a test could not see the flag. That relaxation was invariant along the one axis this PR changes — both URL shapes interpolate the project id — so inverting or deleting the ternary would have stayed green, and it dropped the region coverage cloud depends on. Now fixed.
Pint, PHPStan and Rector clean on all four changed files. Server-ce CI green on VCSGitea and VCSGitHub.
Related PRs and Issues
Follow-up to ea34e44, which moved the commit statuses and the authorize-contributor link onto this flag but left the callbacks behind.
Note for review: the new console has no
settings/git-installationsroute, so/projects/{projectId}/settingsis the closest existing target. Happy to change it if a dedicated page is planned.