Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added sparkpost.php #47

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

vegeta09
Copy link

@vegeta09 vegeta09 commented Oct 8, 2023

What does this PR do?

Pull Request adds a SparkPost messaging adapter.

Test Plan

  1. Create SparkPost account
  2. Set up Sender Domain Information
  3. Set up Recepients Information
  4. Create Api Key
  5. Download this repository & install necessary pacakge by running composer install

->To test with sandbox , read more about it at sparkpost (link - https://developers.sparkpost.com/api/transmissions/#header-the-sandbox-domain)

Run E2E test by replacing your SPARKPOST_API_KEY, SPARKPOST_TO & SPARKPOST_FROM in following command.

SPARKPOST_API_KEY="Your Api Key" SPARKPOST_TO='Receiver's Email' SPARKPOST_FROM="Sender's Email" vendor/bin/phpunit tests/e2e/Email/SparkPostTest.php

Screenshot:

testResult

Related PRs and Issues

Closes: 💬 Improve Appwrite Messaging with SparkPost Adapter

Have you read the Contributing Guidelines on issues?

Yes

@vegeta09 vegeta09 marked this pull request as ready for review October 11, 2023 22:25
Copy link
Member

@PineappleIOnic PineappleIOnic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good overall, just a couple things to keep it within our style and env-var conventions

@@ -40,6 +40,9 @@ jobs:
VONAGE_API_SECRET: ${{ secrets.VONAGE_API_SECRET }}
VONAGE_TO: ${{ secrets.VONAGE_TO }}
VONAGE_FROM: ${{ secrets.VONAGE_FROM }}
SPARKPOST_API_KEY: ${{ secrets.SPARKPOST_API_KEY }}
SPARKPOST_TO: ${{ secrets.SPARKPOST_TO }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you remove SPARKPOST_TO and instead use the TEST_EMAIL Env Var in your code as we already use that in the existing tests. Same goes for SPARKPOST_FROM as we can replace that with the existing TEST_FROM_EMAIL that we also use in tests already

class SparkPost extends EmailAdapter{

public function __construct(private string $apiKey,
private bool $isEu = false){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This formatting looks really weird here, could you run the formatter with composer format?

}

public function getMaxMessagesPerRequest(): int{
//TODO: real value for this
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you find the real value for this or do you want to keep it as 1000?

*/

public function testSendEmail(){

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This formatting is also weird, should be fixed after you run composer format

@gewenyu99
Copy link

Hey,

Due to time constraints, I'm going to mark this PR hacktoberfest-accepted for now so you get DO's Hacktoberfest rewards. We'll continue to work with you on this issue for review and merge.

When it is merged, we'll contact you for Appwrite-specific Hacktoberfest swag.

Thanks for helping us improve Appwrite!

@vegeta09
Copy link
Author

vegeta09 commented Nov 1, 2023

Hey,

Due to time constraints, I'm going to mark this PR hacktoberfest-accepted for now so you get DO's Hacktoberfest rewards. We'll continue to work with you on this issue for review and merge.

When it is merged, we'll contact you for Appwrite-specific Hacktoberfest swag.

Thanks for helping us improve Appwrite!

Thanks @gewenyu99 , for the required changes, i am working on it and will create a PR as soon as they are finished.

@tessamero
Copy link

Hello @vegeta09

Thank you for your contribution to Hacktoberfest 2023! We've noticed that your PR is still pending and requires some updates based on our engineering team's feedback.

We would love to see your PR successfully merged and send you the Appwrite swag as a token of appreciation. To remain eligible for the swag, please address the pending suggestions and/or ensure the tests pass by Friday, November 17th. If the PR isn't updated by then, we will unfortunately have to close it due to the end of the Hacktoberfest event.

Looking forward to your updates and thank you!

@gewenyu99
Copy link

Hey there! There were a lot of big PRs during this Hacktoberfest, and we wanted to give everyone ample time to collaborate with our engineering team. If you were able to merge your PRs during October, amazing. If it’s still not merged, don’t worry about it either. Either way, we’ve got your Hacktoberfest swag minted and ready to ship.

Please comment with your Discord username here so we can contact you about your shipping information to deliver your Hacktoberfest swag.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

💬 Improve Appwrite Messaging with SparkPost Adapter
4 participants