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

Feat 6404 smtp2 go messaging adapter #51

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

Conversation

Ayaan49
Copy link

@Ayaan49 Ayaan49 commented Oct 11, 2023

What does this PR do?

Added SMTP2GO messaging adapter to improve Appwrite messaging.

Test Plan

I created a free SMTP2GO plan, developed the adapter, and tested it by creating a test file inside the "tests" directory. I added the SMTP2GO credentials through environment variables in the Docker Compose file to connect with my SMTP2GO account and verify if emails are being sent. It sent emails successfully. I ran the test case, and it executed without any issues. The command used to run the test case is docker exec -it messaging-tests-1 ./vendor/bin/phpunit tests/e2e/Email/Smtp2goTest.php. All testing was conducted within Docker containers.

Related PRs and Issues

appwrite/appwrite#6404

Have you read the Contributing Guidelines on issues?

Yes

@Ayaan49
Copy link
Author

Ayaan49 commented Oct 31, 2023

Fixed the linting errors @gewenyu99

@@ -24,49 +24,31 @@ public function __construct(
) {
}

/**

Choose a reason for hiding this comment

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

Why did you remove a bunch of comments in unrelated files?

Copy link
Author

Choose a reason for hiding this comment

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

I ran the composer lint command and then it removed the comments to resolve the linting errors issue. @gewenyu99

Choose a reason for hiding this comment

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

????? Really

Copy link
Contributor

Choose a reason for hiding this comment

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

I ran the composer lint command and then it removed the comments to resolve the linting errors issue.

That doesn't quite make sense...composer lint only checks for lint errors. it shouldn't be modifying anything.

Copy link
Author

@Ayaan49 Ayaan49 Nov 2, 2023

Choose a reason for hiding this comment

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

lint error
@stnguyen90 @gewenyu99 Sorry, it was composer format not composer lint. composer format removed those comments because it was causing linting error. However, my files are not causing any linting error.

@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!

@gewenyu99 gewenyu99 requested a review from wess October 31, 2023 23:05
Dockerfile Outdated
Comment on lines 27 to 30
RUN wget -O phpunit https://phar.phpunit.de/phpunit-9.phar && \
chmod +x phpunit && \
mv phpunit /usr/local/bin/phpunit
Copy link
Contributor

Choose a reason for hiding this comment

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

There shouldn't be any need to add this.

composer.json Outdated
Comment on lines 29 to 30
"phpunit/phpunit": "9.6.*",
"phpmailer/phpmailer": "6.8.*",
"phpunit/phpunit": "^9.6",
"phpmailer/phpmailer": "^6.8",
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's change this back to using wildcards. Eventually, we want to use wildcards for everything for clarity.

composer.json Outdated
"Utopia\\Messaging\\": "src/Utopia/Messaging"
"Utopia\\Messaging\\": "src/Utopia/Messaging/"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this changed? We don't have a trailing slash for other libraries 🧐

@@ -24,49 +24,31 @@ public function __construct(
) {
}

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

I ran the composer lint command and then it removed the comments to resolve the linting errors issue.

That doesn't quite make sense...composer lint only checks for lint errors. it shouldn't be modifying anything.

@@ -2,7 +2,12 @@ version: '3.9'

services:
tests:
image: my-php-app
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no need to add this.

Copy link
Author

@Ayaan49 Ayaan49 Nov 2, 2023

Choose a reason for hiding this comment

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

@stnguyen90 @gewenyu99 I have applied the changes suggested by you. The only problem I am facing right now is the linting error caused by those unrelated files. What should I do about it? Composer formatting fixes it by deleting the comments. But is that acceptable?

@Ayaan49
Copy link
Author

Ayaan49 commented Nov 2, 2023

@stnguyen90 @gewenyu99 My tests are succesfull using docker containers. Here is the command
docker exec -it messaging-tests-1 ./vendor/bin/phpunit tests/e2e/Email/Smtp2goTest.php
smtp2go pass appwrite
smtp2go console appwrite

@tessamero
Copy link

Hello @Ayaan49

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!

@Ayaan49
Copy link
Author

Ayaan49 commented Nov 14, 2023

Hello @tessamero , I have already addressed the changes and mentioned the maintainers but I didn't get any reply. Also my tests are passing inside the Docker containers and I have provided ss of the same. Please look into it and tell me if it's ok or not.

@gewenyu99
Copy link

gewenyu99 commented Apr 8, 2024

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.

@Ayaan49
Copy link
Author

Ayaan49 commented Apr 9, 2024

@gewenyu99 ayaan49

@gewenyu99
Copy link

Will try to reach out soon, compiling everyone's names (there's a lot!). Appreciate your patience.

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.

4 participants