Conversation
📝 WalkthroughWalkthroughThis PR extends the migrations system to support JSON exports alongside existing CSV functionality. It generalizes CSV-specific infrastructure by: replacing CSV export email translation keys with generic data export keys in the translation file, introducing new API endpoints at Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
commit: |
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@app/config/locale/translations/en.json`:
- Around line 60-74: The failure email signature uses a different placeholder
than the success template; update the emails.dataExport.failure.signature entry
to use the same placeholder as emails.dataExport.success.signature (replace
{{project}} with {{platform}}) so both success
(emails.dataExport.success.signature) and failure
(emails.dataExport.failure.signature) templates render a consistent sign-off.
In `@app/controllers/api/migrations.php`:
- Around line 734-738: The queue setup mistakenly calls setProject twice and
never sets the platform, so update the chain on $queueForMigrations to replace
the duplicate setProject($project) with setPlatform($platform) (keeping the
existing setMigration($migration) and setProject($project) calls) before
->trigger(); so the worker receives the correct $platform value.
🧹 Nitpick comments (1)
src/Appwrite/Platform/Workers/Migrations.php (1)
518-519: Consider adding a null guard for$dataExportType.While the current call flow guarantees
$dataExportTypeis non-null when this code executes (becausehandleDataExportCompleteis only called for CSV/JSON destinations), adding an explicit guard would make the code more defensive and clearer.🔧 Optional: Add defensive null check
+ if ($this->dataExportType === null) { + throw new \Exception('Data export type not set'); + } $ext = strtolower($this->dataExportType); $path = $this->deviceForFiles->getPath($bucketId . '/' . $this->sanitizeFilename($filename) . '.' . $ext);
| "emails.dataExport.success.subject": "Your {{type}} export is ready", | ||
| "emails.dataExport.success.preview": "Your data export has been completed successfully.", | ||
| "emails.dataExport.success.hello": "Hello {{user}},", | ||
| "emails.dataExport.success.body": "Your {{type}} export is ready to download. Click the button below to download your data export.", | ||
| "emails.dataExport.success.footer": "This download link will expire in 1 hour.", | ||
| "emails.dataExport.success.thanks": "Thanks,", | ||
| "emails.dataExport.success.buttonText": "Download {{type}}", | ||
| "emails.dataExport.success.signature": "{{platform}} team", | ||
| "emails.dataExport.failure.subject": "Your {{type}} export failed - file too large", | ||
| "emails.dataExport.failure.preview": "Your data export failed because the file size exceeds your plan limit.", | ||
| "emails.dataExport.failure.hello": "Hello {{user}},", | ||
| "emails.dataExport.failure.body": "Your {{type}} export could not be completed because the export file size ({{size}}MB) exceeds your plan limit. Please consider upgrading your plan or exporting a smaller dataset.", | ||
| "emails.dataExport.failure.footer": "If you have any questions, please contact our support team.", | ||
| "emails.dataExport.failure.thanks": "Thanks,", | ||
| "emails.dataExport.failure.signature": "{{project}} team", |
There was a problem hiding this comment.
Inconsistent placeholder in signature between success and failure templates.
The success template uses {{platform}} for the signature (line 67), while the failure template uses {{project}} (line 74). This will result in different sign-offs:
- Success: "Appwrite team" (platform name)
- Failure: "MyProject team" (project name)
This should likely be consistent. Consider using {{platform}} for both to match the success template.
🔧 Suggested fix
- "emails.dataExport.failure.signature": "{{project}} team",
+ "emails.dataExport.failure.signature": "{{platform}} team",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "emails.dataExport.success.subject": "Your {{type}} export is ready", | |
| "emails.dataExport.success.preview": "Your data export has been completed successfully.", | |
| "emails.dataExport.success.hello": "Hello {{user}},", | |
| "emails.dataExport.success.body": "Your {{type}} export is ready to download. Click the button below to download your data export.", | |
| "emails.dataExport.success.footer": "This download link will expire in 1 hour.", | |
| "emails.dataExport.success.thanks": "Thanks,", | |
| "emails.dataExport.success.buttonText": "Download {{type}}", | |
| "emails.dataExport.success.signature": "{{platform}} team", | |
| "emails.dataExport.failure.subject": "Your {{type}} export failed - file too large", | |
| "emails.dataExport.failure.preview": "Your data export failed because the file size exceeds your plan limit.", | |
| "emails.dataExport.failure.hello": "Hello {{user}},", | |
| "emails.dataExport.failure.body": "Your {{type}} export could not be completed because the export file size ({{size}}MB) exceeds your plan limit. Please consider upgrading your plan or exporting a smaller dataset.", | |
| "emails.dataExport.failure.footer": "If you have any questions, please contact our support team.", | |
| "emails.dataExport.failure.thanks": "Thanks,", | |
| "emails.dataExport.failure.signature": "{{project}} team", | |
| "emails.dataExport.success.subject": "Your {{type}} export is ready", | |
| "emails.dataExport.success.preview": "Your data export has been completed successfully.", | |
| "emails.dataExport.success.hello": "Hello {{user}},", | |
| "emails.dataExport.success.body": "Your {{type}} export is ready to download. Click the button below to download your data export.", | |
| "emails.dataExport.success.footer": "This download link will expire in 1 hour.", | |
| "emails.dataExport.success.thanks": "Thanks,", | |
| "emails.dataExport.success.buttonText": "Download {{type}}", | |
| "emails.dataExport.success.signature": "{{platform}} team", | |
| "emails.dataExport.failure.subject": "Your {{type}} export failed - file too large", | |
| "emails.dataExport.failure.preview": "Your data export failed because the file size exceeds your plan limit.", | |
| "emails.dataExport.failure.hello": "Hello {{user}},", | |
| "emails.dataExport.failure.body": "Your {{type}} export could not be completed because the export file size ({{size}}MB) exceeds your plan limit. Please consider upgrading your plan or exporting a smaller dataset.", | |
| "emails.dataExport.failure.footer": "If you have any questions, please contact our support team.", | |
| "emails.dataExport.failure.thanks": "Thanks,", | |
| "emails.dataExport.failure.signature": "{{platform}} team", |
🤖 Prompt for AI Agents
In `@app/config/locale/translations/en.json` around lines 60 - 74, The failure
email signature uses a different placeholder than the success template; update
the emails.dataExport.failure.signature entry to use the same placeholder as
emails.dataExport.success.signature (replace {{project}} with {{platform}}) so
both success (emails.dataExport.success.signature) and failure
(emails.dataExport.failure.signature) templates render a consistent sign-off.
| $queueForMigrations | ||
| ->setMigration($migration) | ||
| ->setProject($project) | ||
| ->setProject($project) | ||
| ->trigger(); |
There was a problem hiding this comment.
Fix queue setup: platform is never set.
The second setProject($project) looks like a copy/paste mistake. This leaves $platform unset for the worker and can break the migration flow.
🐛 Suggested fix
$queueForMigrations
->setMigration($migration)
->setProject($project)
- ->setProject($project)
+ ->setPlatform($platform)
->trigger();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $queueForMigrations | |
| ->setMigration($migration) | |
| ->setProject($project) | |
| ->setProject($project) | |
| ->trigger(); | |
| $queueForMigrations | |
| ->setMigration($migration) | |
| ->setProject($project) | |
| ->setPlatform($platform) | |
| ->trigger(); |
🤖 Prompt for AI Agents
In `@app/controllers/api/migrations.php` around lines 734 - 738, The queue setup
mistakenly calls setProject twice and never sets the platform, so update the
chain on $queueForMigrations to replace the duplicate setProject($project) with
setPlatform($platform) (keeping the existing setMigration($migration) and
setProject($project) calls) before ->trigger(); so the worker receives the
correct $platform value.
✨ Benchmark results
⚡ Benchmark Comparison
|
1 participant
What does this PR do?
Import, Export with JSON.
Test Plan
E2E
Related PRs and Issues
Checklist