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

Support Azure Communication SMS #15539

Merged
merged 34 commits into from
Sep 16, 2024
Merged

Support Azure Communication SMS #15539

merged 34 commits into from
Sep 16, 2024

Conversation

hishamco
Copy link
Member

@hishamco hishamco commented Mar 18, 2024

Adds support for Azure Communication SMS

@MikeAlhayek
Copy link
Member

Way too many files changes here. It seems that you copied all the SMS files into a new module. All you should need to do

  1. Create OC.Sms.Azure module project
  2. Implement the ISmsProvider and register it as described here https://docs.orchardcore.net/en/latest/docs/reference/modules/Sms/
  3. You may want to add a provider for configuration provider using DefaultAzure key and another for Settings Azure. This way user can use a saas environment provider "if available" or they can enable one by providing their custom Azure instance settings like we did in the SMTP and Azure Email providers.

@hishamco
Copy link
Member Author

As I said it's WIP, I'm working on it :)

@agriffard agriffard changed the title Suuport Azure Communication SMS Support Azure Communication SMS Mar 19, 2024
Copy link
Contributor

github-actions bot commented Apr 7, 2024

This pull request has merge conflicts. Please resolve those before requesting a review.

Copy link
Contributor

github-actions bot commented Jun 8, 2024

It seems that this pull request didn't really move for quite a while. Is this something you'd like to revisit any time soon or should we close? Please comment if you'd like to pick it up.

@github-actions github-actions bot added the stale label Jun 8, 2024
Copy link
Contributor

Closing this pull request because it has been stale for very long. If you think this is still relevant, feel free to reopen it.

@github-actions github-actions bot closed this Jun 23, 2024
@hishamco hishamco reopened this Jun 24, 2024
@github-actions github-actions bot removed the stale label Jun 25, 2024
@MikeAlhayek
Copy link
Member

for instructions on how to resolve the merge conflicts due to #16572 please follow the step listed in this comment.

@hishamco hishamco marked this pull request as ready for review September 13, 2024 17:19
@hishamco
Copy link
Member Author

@MikeAlhayek could you please do a quick review

@hishamco
Copy link
Member Author

I suggest moving the Twilio-related types into OrchardCore.Sms.Twilio to follow the structure that we have before

Copy link
Member

@MikeAlhayek MikeAlhayek left a comment

Choose a reason for hiding this comment

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

@hishamco here are some feedback. Also, please remove the template file changes from this PR.

Twilio provider should be kept in the Sms module.

@hishamco
Copy link
Member Author

Copy link
Member

@MikeAlhayek MikeAlhayek left a comment

Choose a reason for hiding this comment

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

@hishamco now you created naming inconsistency between UI and documentation. Please use "Azure Communication SMS" everywhere as the suggestions below. This will keep naming consistency, and allow us to add another SMS provider offered by Azure later when they decide to introduce a new service.

src/docs/reference/modules/Sms.Azure/README.md Outdated Show resolved Hide resolved
src/docs/reference/modules/Sms.Azure/README.md Outdated Show resolved Hide resolved
src/docs/reference/modules/Sms.Azure/README.md Outdated Show resolved Hide resolved
src/docs/reference/modules/Sms.Azure/README.md Outdated Show resolved Hide resolved
src/docs/reference/modules/Sms.Azure/README.md Outdated Show resolved Hide resolved
src/docs/reference/modules/Sms/README.md Show resolved Hide resolved
src/docs/releases/2.1.0.md Outdated Show resolved Hide resolved
src/docs/reference/modules/Sms.Azure/README.md Outdated Show resolved Hide resolved
@MikeAlhayek MikeAlhayek dismissed their stale review September 15, 2024 18:43

The PR changed since the approval

Copy link
Member

@MikeAlhayek MikeAlhayek left a comment

Choose a reason for hiding this comment

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

@hishamco
Can you please search for the docs and the code for anything that say Azure Communication SMS and replace it with *Azure Communication. Because when we are in the SMS UI, the term SMS is implied. So we should not repeat it everywhere specially in the UI.

mkdocs.yml Outdated Show resolved Hide resolved
src/docs/reference/README.md Outdated Show resolved Hide resolved
@hishamco
Copy link
Member Author

Can you please search for the docs and the code for anything that say Azure Communication SMS and replace it with *Azure Communication.

Nothing except this docs

@hishamco
Copy link
Member Author

If it's ok I will merge then

Copy link
Member

@MikeAlhayek MikeAlhayek left a comment

Choose a reason for hiding this comment

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

I thought you did the string/replace.

@hishamco
Copy link
Member Author

I looked for what you asking for "Azure Communication SMS" not "Azure SMS" :)

@MikeAlhayek MikeAlhayek enabled auto-merge (squash) September 15, 2024 23:57
@MikeAlhayek MikeAlhayek merged commit 4f4bec4 into main Sep 16, 2024
11 of 12 checks passed
@MikeAlhayek MikeAlhayek deleted the hishamco/azure-sms branch September 16, 2024 00:02
@MikeAlhayek
Copy link
Member

/backport to release/2.0

Copy link
Contributor

Started backporting to release/2.0: https://github.com/OrchardCMS/OrchardCore/actions/runs/11782162720

@hishamco
Copy link
Member Author

The first time I saw such BOT when it was introduced.

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

Successfully merging this pull request may close these issues.

3 participants