Feat fix 308 redirect #9553
Feat fix 308 redirect #9553
Conversation
WalkthroughThis change introduces a new end-to-end test method Changes
Sequence Diagram(s)sequenceDiagram
participant T as Test Runner
participant S as SitesCustomServerTest
participant Site as Site Engine
participant Proxy as Proxy Client
T->>S: Execute testPermanentRedirect()
S->>Site: Initialize site with parameters
Site-->>S: Return site created (HTTP 200)
S->>Site: Setup domain and create deployment
Site-->>S: Deployment created (HTTP 200)
S->>Proxy: GET request to "/"
Proxy-->>S: 200 OK response
S->>Proxy: GET request to "/project1/"
Proxy-->>S: 200 OK response
S->>T: Cleanup site and conclude test
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
🧰 Additional context used🧬 Code Definitions (1)tests/e2e/Services/Sites/SitesCustomServerTest.php (2)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/e2e/Services/Sites/SitesCustomServerTest.php (1)
2334-2370: LGTM! Well-implemented test for permanent redirect functionality.The test method is well-structured, properly following the established testing patterns in this class. It appropriately:
- Sets up a site with required parameters
- Verifies site creation
- Sets up domain and deployment
- Tests relevant HTTP responses for different paths
- Verifies the 308 status code for the trailing slash redirect
- Cleans up resources after testing
Consider enhancing the test by also verifying the
Locationheader in the redirect response to ensure it points to the correct destination URL. This would provide more complete validation of the redirect behavior.- $response2 = $proxyClient->call(Client::METHOD_GET, '/project1/'); - $this->assertEquals(308, $response2['headers']['status-code']); + $response2 = $proxyClient->call(Client::METHOD_GET, '/project1/'); + $this->assertEquals(308, $response2['headers']['status-code']); + $this->assertArrayHasKey('location', $response2['headers']); + $this->assertEquals('/project1', $response2['headers']['location']);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/e2e/Services/Sites/SitesCustomServerTest.php(1 hunks)tests/resources/sites/subProjects/index.html(1 hunks)tests/resources/sites/subProjects/project1/index.html(1 hunks)tests/resources/sites/subProjects/project2/index.html(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
tests/e2e/Services/Sites/SitesCustomServerTest.php (3)
tests/e2e/Services/Sites/SitesBase.php (4) (4)
setupSite(20-33)getSite(161-169)setupSiteDomain(329-347)setupDeployment(35-67)tests/e2e/Services/Functions/FunctionsBase.php (1) (1)
setupDeployment(35-67)src/Executor/Executor.php (1) (1)
call(290-416)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (3)
tests/resources/sites/subProjects/project1/index.html (1)
1-13: LGTM! Well-structured HTML file for Project 1.The HTML structure is clean and correctly formatted with all necessary elements - proper doctype, meta tags for charset and viewport, appropriate title, and a well-organized body with heading, welcome text, and navigation link back to the main page.
tests/resources/sites/subProjects/project2/index.html (1)
1-13: LGTM! Well-structured HTML file for Project 2.The HTML structure is clean and correctly formatted with all necessary elements - proper doctype, meta tags for charset and viewport, appropriate title, and a well-organized body with heading, welcome text, and navigation link back to the main page.
tests/resources/sites/subProjects/index.html (1)
1-17: LGTM! Well-structured main index HTML file.The HTML structure is clean and correctly formatted with appropriate navigation to the sub-projects. The use of an unordered list for project links provides good semantic structure.
✨ Benchmark results
⚡ Benchmark Comparison
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/e2e/Services/Sites/SitesCustomServerTest.php (4)
2397-2432: LGTM for basic redirect testing, consider enhanced assertions for 308 redirect validation.The test successfully verifies that both paths ('/project1' and '/project1/') return 200 status codes, confirming basic redirect functionality is working. However, if the specific goal is to test 308 permanent redirects, consider improving the test with the following enhancements:
- Add redirect-specific assertions by setting
followRedirects: falseto observe the actual redirect status code:-$response1 = $proxyClient->call(Client::METHOD_GET, '/project1'); -$this->assertEquals(200, $response1['headers']['status-code']); +$response1 = $proxyClient->call(Client::METHOD_GET, '/project1', followRedirects: false); +$this->assertEquals(308, $response1['headers']['status-code']); +$this->assertArrayHasKey('location', $response1['headers']); +$this->assertEquals('/project1/', $response1['headers']['location']); + +// Test the redirected URL directly +$response1Redirected = $proxyClient->call(Client::METHOD_GET, '/project1/'); +$this->assertEquals(200, $response1Redirected['headers']['status-code']);
- Consider checking response body content to verify the correct page is loaded:
$response2 = $proxyClient->call(Client::METHOD_GET, '/project1/'); $this->assertEquals(200, $response2['headers']['status-code']); +$this->assertStringContainsString('Project 1', $response2['body']);< 8000 /span>
2410-2410: Use integer for status code comparison instead of string.For consistency with the rest of the codebase, use an integer value (200) instead of a string ('200') when comparing status codes.
-$this->assertEquals('200', $site['headers']['status-code']); +$this->assertEquals(200, $site['headers']['status-code']);
2417-2417: Use boolean instead of string for 'activate' parameter.For consistency with other places in the codebase, consider using a boolean
trueinstead of the string'true'.- 'activate' => 'true' + 'activate' => true
2421-2421: Remove unused site retrieval or add assertions.This line retrieves the site details but doesn't use the returned value. Either remove it or add assertions to verify expected site properties after deployment.
-$site = $this->getSite($siteId); +// Either remove this line or add assertions, for example: +$site = $this->getSite($siteId); +$this->assertNotEmpty($site['body']['deploymentId']); +$this->assertEquals($deploymentId, $site['body']['deploymentId']);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
tests/e2e/Services/Sites/SitesCustomServerTest.php(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Setup & Build Appwrite Image
What does this PR do?
Test for 308 redirect
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Screenshots may also be helpful.)
Related PRs and Issues
Checklist
Summary by CodeRabbit