Skip deployment when commit is created by us #10187
Conversation
📝 WalkthroughWalkthroughThis change introduces two new constants, Estimated code review effort2 (~15 minutes) Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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! |
Meldiron
left a comment
There was a problem hiding this comment.
LGTM, we just need to update version in composer.json once we release DB lib version
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/e2e/Services/Databases/DatabasesBase.php (1)
2000-2002: Consider using an early return for better readabilityInstead of wrapping the entire test logic in a conditional block, consider using an early return pattern for improved readability.
- if ($this->getSide() === 'client') { - // Skipped on server side: Creating a document with no permissions results in an empty permissions array, whereas on client side it assigns permissions to the current user + if ($this->getSide() !== 'client') { + // Skipped on server side: Creating a document with no permissions results in an empty permissions array, whereas on client side it assigns permissions to the current user + return; + }Then un-indent the entire block that follows.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/controllers/api/databases.php(2 hunks)app/controllers/api/vcs.php(2 hunks)tests/e2e/Services/Databases/DatabasesBase.php(1 hunks)
🧠 Learnings (2)
tests/e2e/Services/Databases/DatabasesBase.php (1)
Learnt from: ItzNotABug
PR: #9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php:57-59
Timestamp: 2025-06-19T09:20:03.312Z
Learning: In table-related endpoints (such as src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php), parameter descriptions should use "table" and "row" terminology instead of "collection" and "document" for clarity and consistency.
app/controllers/api/databases.php (1)
Learnt from: ItzNotABug
PR: #9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php:57-59
Timestamp: 2025-06-19T09:20:03.312Z
Learning: In table-related endpoints (such as src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php), parameter descriptions should use "table" and "row" terminology instead of "collection" and "document" for clarity and consistency.
🚧 Files skipped from review as they are similar to previous changes (1)
- app/controllers/api/vcs.php
🧰 Additional context used
🧠 Learnings (2)
tests/e2e/Services/Databases/DatabasesBase.php (1)
Learnt from: ItzNotABug
PR: #9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php:57-59
Timestamp: 2025-06-19T09:20:03.312Z
Learning: In table-related endpoints (such as src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php), parameter descriptions should use "table" and "row" terminology instead of "collection" and "document" for clarity and consistency.
app/controllers/api/databases.php (1)
Learnt from: ItzNotABug
PR: #9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php:57-59
Timestamp: 2025-06-19T09:20:03.312Z
Learning: In table-related endpoints (such as src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php), parameter descriptions should use "table" and "row" terminology instead of "collection" and "document" for clarity and consistency.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: scan
🔇 Additional comments (5)
app/controllers/api/databases.php (2)
4260-4260: LGTM: User injection added appropriately.The user document injection is correctly added to support the permissions handling logic below. This enables the endpoint to create default permissions for the current user when none are explicitly provided.
4284-4305: Well-implemented permissions inheritance logic.This change ensures permissions are never null during upsert operations by:
- Create scenario: When no existing document is found, it assigns default permissions (read, update, delete) to the current user
- Update scenario: When an existing document is found, it inherits the existing permissions
- Security consideration: Only creates default permissions when a valid user ID exists
The implementation correctly uses
Authorization::skip()for internal document retrieval and properly constructsPermissionobjects. This addresses the core issue mentioned in the AI summary about ensuring permissions are never null during upsert operations.tests/e2e/Services/Databases/DatabasesBase.php (3)
2019-2026: Good use of assertEqualsCanonicalizing for permission comparisonThe test correctly validates that default permissions are created when none are provided during upsert on the client side. Using
assertEqualsCanonicalizingis the right choice here since the order of permissions doesn't matter.
2050-2099: Comprehensive permission enforcement testingExcellent test coverage for permission modifications:
- Correctly tests removal of delete permission
- Properly asserts 401 status code for unauthorized delete attempts
- Validates permission restoration and successful deletion
2101-2152: Thorough testing of permission inheritance for related documentsThe test comprehensively validates that:
- Related documents created during upsert also receive default permissions
- Both parent and child documents get the expected 3 permissions
- Documents can be queried and retrieved with proper permissions
- Direct access to related documents shows correct permission inheritance
Well-structured test for complex relationship scenarios.
What does this PR do?
Skip triggering a deployment in webhooks flow when commit is created by us.
Test Plan
Related PRs and Issues