Feat: list email templates#11956
Conversation
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…-api-with-list-endpoint
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Greptile SummaryThis PR adds a
Confidence Score: 3/5Not safe to merge — the list endpoint exposes an empty Two P1 findings share the same root cause:
Important Files Changed
Reviews (1): Last reviewed commit: "Add email template list response model" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| $template['custom'] = true; | ||
| } | ||
|
|
||
| $template['type'] = $type; |
There was a problem hiding this comment.
type vs templateId
The list endpoint stores the template type under the type key, but the TemplateEmail model defines templateId as its field (with a default of ''). The Get endpoint correctly uses $template['templateId'] = $templateId. This means templateId will always serialize as an empty string in list responses, and testListEmailTemplatesDefaultMatchesGet will fail because $get['body']['type'] is null (GET doesn't set a type attribute) while $listed['type'] is non-null.
| $template['type'] = $type; | |
| $template['templateId'] = $type; |
| $templates = []; | ||
| foreach ($types as $type) { | ||
| foreach ($localeCodes as $locale) { | ||
| $key = 'email.' . $type . '-' . $locale; | ||
| $stored = $projectTemplates[$key] ?? null; | ||
|
|
||
| $localeObj = new Locale($locale); | ||
| $localeObj->setFallback(System::getEnv('_APP_LOCALE', 'en')); | ||
|
|
||
| if (is_null($stored)) { | ||
| /** | ||
| * different templates, different placeholders. | ||
| */ | ||
| $templateConfigs = [ | ||
| 'magicSession' => [ | ||
| 'file' => 'email-magic-url.tpl', | ||
| 'placeholders' => ['optionButton', 'buttonText', 'optionUrl', 'clientInfo', 'securityPhrase'] | ||
| ], | ||
| 'mfaChallenge' => [ | ||
| 'file' => 'email-mfa-challenge.tpl', | ||
| 'placeholders' => ['description', 'clientInfo'] | ||
| ], | ||
| 'otpSession' => [ | ||
| 'file' => 'email-otp.tpl', | ||
| 'placeholders' => ['description', 'clientInfo', 'securityPhrase'] | ||
| ], | ||
| 'sessionAlert' => [ | ||
| 'file' => 'email-session-alert.tpl', | ||
| 'placeholders' => ['body', 'listDevice', 'listIpAddress', 'listCountry', 'footer'] | ||
| ], | ||
| ]; | ||
|
|
||
| // fallback to the base template. | ||
| $config = $templateConfigs[$type] ?? [ | ||
| 'file' => 'email-inner-base.tpl', | ||
| 'placeholders' => ['buttonText', 'body', 'footer'] | ||
| ]; | ||
|
|
||
| $templateString = file_get_contents(APP_CE_CONFIG_DIR . '/locale/templates/' . $config['file']); | ||
|
|
||
| // We use `fromString` due to the replace above | ||
| $message = Template::fromString($templateString); | ||
|
|
||
| // Set type-specific parameters | ||
| foreach ($config['placeholders'] as $param) { | ||
| $escapeHtml = !in_array($param, ['clientInfo', 'body', 'footer', 'description']); | ||
| $message->setParam("{{{$param}}}", $localeObj->getText("emails.{$type}.{$param}"), escapeHtml: $escapeHtml); | ||
| } | ||
|
|
||
| $message | ||
| // common placeholders on all the templates | ||
| ->setParam('{{hello}}', $localeObj->getText("emails.{$type}.hello")) | ||
| ->setParam('{{thanks}}', $localeObj->getText("emails.{$type}.thanks")) | ||
| ->setParam('{{signature}}', $localeObj->getText("emails.{$type}.signature")); | ||
|
|
||
| // `useContent: false` will strip new lines! | ||
| $message = $message->render(useContent: true); | ||
|
|
||
| $template = [ | ||
| 'message' => $message, | ||
| 'subject' => $localeObj->getText('emails.' . $type . '.subject'), | ||
| 'senderEmail' => '', | ||
| 'senderName' => '', | ||
| 'custom' => false, | ||
| ]; | ||
| } else { | ||
| $template = $stored; | ||
| $template['custom'] = true; | ||
| } | ||
|
|
||
| $template['type'] = $type; | ||
| $template['locale'] = $locale; | ||
|
|
||
| $templates[] = new Document($template); | ||
| } | ||
| } |
There was a problem hiding this comment.
Every request renders all types × localeCodes templates — including file_get_contents() and full HTML rendering — before any filter or pagination is applied. With a large locale code set this can mean hundreds of disk reads and template renders for a request that only needs a few results. Consider applying type/locale filters first (which can be extracted cheaply from the query before rendering) and rendering lazily, or at minimum caching the template file strings outside the inner loop.
| $this->assertSame($get['body']['type'], $listed['type']); | ||
| $this->assertSame($get['body']['locale'], $listed['locale']); | ||
| $this->assertSame($get['body']['custom'], $listed['custom']); | ||
| $this->assertSame($get['body']['subject'], $listed['subject']); | ||
| $this->assertSame($get['body']['message'], $listed['message']); | ||
| $this->assertSame($get['body']['senderName'], $listed['senderName']); | ||
| $this->assertSame($get['body']['senderEmail'], $listed['senderEmail']); | ||
| $this->assertSame($get['body']['replyTo'], $listed['replyTo']); |
There was a problem hiding this comment.
type from GET response that doesn't set type
testListEmailTemplatesDefaultMatchesGet accesses $get['body']['type'] on line 226, but the GET endpoint stores the field as templateId (not type) in the document and serializes it through the TemplateEmail model which has a templateId rule — there is no type attribute in the GET response. $get['body']['type'] will be null, so assertSame(null, 'verification') will fail. Once the list endpoint is corrected to use templateId, this comparison should be updated to use templateId as well:
| $this->assertSame($get['body']['type'], $listed['type']); | |
| $this->assertSame($get['body']['locale'], $listed['locale']); | |
| $this->assertSame($get['body']['custom'], $listed['custom']); | |
| $this->assertSame($get['body']['subject'], $listed['subject']); | |
| $this->assertSame($get['body']['message'], $listed['message']); | |
| $this->assertSame($get['body']['senderName'], $listed['senderName']); | |
| $this->assertSame($get['body']['senderEmail'], $listed['senderEmail']); | |
| $this->assertSame($get['body']['replyTo'], $listed['replyTo']); | |
| $this->assertSame($get['body']['templateId'], $listed['templateId']); | |
| $this->assertSame($get['body']['locale'], $listed['locale']); | |
| $this->assertSame($get['body']['custom'], $listed['custom']); | |
| $this->assertSame($get['body']['subject'], $listed['subject']); | |
| $this->assertSame($get['body']['message'], $listed['message']); | |
| $this->assertSame($get['body']['senderName'], $listed['senderName']); | |
| $this->assertSame($get['body']['senderEmail'], $listed['senderEmail']); | |
| $this->assertSame($get['body']['replyTo'], $listed['replyTo']); |
🔄 PHP-Retry SummaryFlaky tests detected across commits: Commit
|
| Test | Retries | Total Time | Details |
|---|---|---|---|
UsageTest::testFunctionsStats |
1 | 10.26s | Logs |
UsageTest::testPrepareSitesStats |
1 | 7ms | Logs |
UsageTest::testEmbeddingsTextUsageDoesNotBreakProjectUsage |
1 | 6ms | Logs |
WebhooksCustomServerTest::testDeleteDeployment |
1 | 13ms | Logs |
WebhooksCustomServerTest::testDeleteFunction |
1 | 7ms | Logs |
WebhooksCustomServerTest::testCreateCollection |
1 | 8ms | Logs |
WebhooksCustomServerTest::testCreateAttributes |
1 | 9ms | Logs |
WebhooksCustomServerTest::testCreateDocument |
1 | 37ms | Logs |
WebhooksCustomServerTest::testUpdateDocument |
1 | 18ms | Logs |
WebhooksCustomServerTest::testDeleteDocument |
1 | 13ms | Logs |
WebhooksCustomServerTest::testCreateTable |
1 | 11ms | Logs |
WebhooksCustomServerTest::testCreateColumns |
1 | 73ms | Logs |
WebhooksCustomServerTest::testCreateRow |
1 | 19ms | Logs |
WebhooksCustomServerTest::testUpdateRow |
1 | 15ms | Logs |
WebhooksCustomServerTest::testDeleteRow |
1 | 17ms | Logs |
WebhooksCustomServerTest::testCreateStorageBucket |
1 | 7ms | Logs |
WebhooksCustomServerTest::testUpdateStorageBucket |
1 | 15ms | Logs |
WebhooksCustomServerTest::testCreateBucketFile |
1 | 14ms | Logs |
WebhooksCustomServerTest::testUpdateBucketFile |
1 | 16ms | Logs |
WebhooksCustomServerTest::testDeleteBucketFile |
1 | 7ms | Logs |
WebhooksCustomServerTest::testDeleteStorageBucket |
1 | 14ms | Logs |
WebhooksCustomServerTest::testCreateTeam |
1 | 5ms | Logs |
WebhooksCustomServerTest::testUpdateTeam |
1 | 13ms | Logs |
WebhooksCustomServerTest::testUpdateTeamPrefs |
1 | 15ms | Logs |
WebhooksCustomServerTest::testDeleteTeam |
1 | 4ms | Logs |
WebhooksCustomServerTest::testCreateTeamMembership |
1 | 12ms | Logs |
WebhooksCustomServerTest::testDeleteTeamMembership |
1 | 10ms | Logs |
WebhooksCustomServerTest::testWebhookAutoDisable |
1 | 24ms | Logs |
✨ Benchmark results
⚡ Benchmark Comparison
|
|
Found 1 test failure on Blacksmith runners: Failure
|
1 participant
Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com<!--
Thank you for sending the PR! We appreciate you spending the time to work on these changes.
Help us understand your motivation by explaining why you decided to make this change.
You can learn more about contributing to appwrite here: https://github.com/appwrite/appwrite/blob/master/CONTRIBUTING.md
Happy contributing!
-->
What does this PR do?
(Provide a description of what this PR does and why it's needed.)
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