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-6861-46elks-messaging-adapter #54

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

Conversation

DhairyaMajmudar
Copy link

@DhairyaMajmudar DhairyaMajmudar commented Oct 11, 2023

What does this PR do?

Integration of 46elks SMS to the Appwrite messaging system

Test Plan

Added a new 46elk.php file in src/Utpoia/Messaging/Adapters/SMS folder path . In this file I have added the PHP code for the integration of 46elks SMS with the Appwrite messaging system.

Related PRs and Issues

Closes appwrite/appwrite#6861

Have you read the Contributing Guidelines on issues?

YES

@DhairyaMajmudar
Copy link
Author

@wess Thank you for approving my PR . Just wanted to ask when it will be merged in the codebase.

@DhairyaMajmudar
Copy link
Author

Discord Handle :- dhairya7#5015

@tessamero
Copy link

Hello @DhairyaMajmudar

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 failed checks and update your PR and also show a screenshot of your tests passing 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!

@DhairyaMajmudar
Copy link
Author

Hey @wess I was trying to fix the failed lint and e2e checks but didn't got the issues , can pls. point to the issue failing these checks
Thanks!

Copy link

@2002Bishwajeet 2002Bishwajeet left a comment

Choose a reason for hiding this comment

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

Rest looks fine although the lint might be complaining about the casing in the file name.

Would Recommend to rename the file like (FortySixelks.php)

To Fix the remaining lint errors:

run composer format

Tests are failing and its not related to your issue.

Although I would recommend to add a test file for this adaptor as well.
It's simple and you can refer to one of the adaptors here

src/Utopia/Messaging/Adapters/SMS/46elks.php Outdated Show resolved Hide resolved
@DhairyaMajmudar
Copy link
Author

@2002Bishwajeet I have committed suggested changes and also added the test files for testing
I have also runned the lint checking command composer lint and the tests were passing.

Pls. review this and merge my PR
Thanks !

Screenshot from 2023-11-18 08-57-52

@DhairyaMajmudar
Copy link
Author

@wess @2002Bishwajeet pls. review my pr and merge it .

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

@DhairyaMajmudar
Copy link
Author

Thank you @gewenyu99 here is my discord username dhairya7

@gewenyu99
Copy link

We'll contact again when we reach out. Adding your name to the list! Appreciate your immense patience with us.

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 46elks
5 participants