Skip to content

[Notifier] add Mailjet SMS bridge #41705

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 4, 2021
Merged

[Notifier] add Mailjet SMS bridge #41705

merged 1 commit into from
Jul 4, 2021

Conversation

jnadaud
Copy link
Contributor

@jnadaud jnadaud commented Jun 14, 2021 •
edited by OskarStark
Loading

Q A
Branch? 5.4
Bug fix? no
New feature? yes
Deprecations? no
License MIT
Doc symfony/symfony-docs#15436

Adding Mailjet SMS bridge for Notifier component

@carsonbot
Copy link

Hey!

To help keep things organized, we don't allow "Draft" pull requests. Could you please click the "ready for review" button or close this PR and open a new one when you are done?

Note that a pull request does not have to be "perfect" or "ready for merge" when you first open it. We just want it to be ready for a first review.

Cheers!

Carsonbot

@jnadaud jnadaud mentioned this pull request Jun 14, 2021
Closed
@jnadaud jnadaud force-pushed the mailjet_notifier branch from 4929c4d to f9fc518 Compare June 14, 2021 17:51
@jnadaud jnadaud marked this pull request as ready for review June 14, 2021 18:06
@jnadaud jnadaud requested a review from OskarStark as a code owner June 14, 2021 18:06
@carsonbot
Copy link

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

  • Always add tests
  • Keep backward compatibility (see https://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
  • Features and deprecations must be submitted against the 5.4 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

@jnadaud jnadaud force-pushed the mailjet_notifier branch 2 times, most recently from 47803d2 to 555a2d5 Compare June 14, 2021 18:53
@jnadaud jnadaud force-pushed the mailjet_notifier branch from 555a2d5 to 62687b5 Compare June 15, 2021 02:16
@nicolas-grekas nicolas-grekas modified the milestones: 6.0, 5.4 Jun 15, 2021
@OskarStark
Copy link
Contributor

It looks, like from is required and user/password is not used, I would use from as user and authtoken as pass for the DSN, like explained here: #41522 (comment)

@jnadaud jnadaud force-pushed the mailjet_notifier branch from 1a0cc4a to ca85797 Compare June 16, 2021 16:08
@jnadaud
Copy link
Contributor Author

jnadaud commented Jun 16, 2021

@OskarStark Done!!!

@jnadaud jnadaud force-pushed the mailjet_notifier branch from ca85797 to 9e616c7 Compare June 16, 2021 16:14
OskarStark
OskarStark reviewed Jun 17, 2021
View reviewed changes
src/Symfony/Component/Notifier/Bridge/Mailjet/MailjetTransport.php Outdated

public function __toString(): string
{
return sprintf('mailjet://%s:%s@%s', $this->from, $this->authToken, $this->getEndpoint());
Copy link
Contributor

@OskarStark OskarStark Jun 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The auth-token should not be exposed, please remove it and fix the tests, thanks

@fabpot
Copy link
Member

fabpot commented Jun 23, 2021

@jnadaud Can you make the last change asked by Oskar? Thank you.

@fabpot fabpot force-pushed the mailjet_notifier branch from 9e616c7 to ee744e7 Compare July 4, 2021 09:06
@fabpot
Copy link
Member

fabpot commented Jul 4, 2021

Thank you @jnadaud.

@fabpot fabpot merged commit 42790be into symfony:5.4 Jul 4, 2021
@fabpot
Copy link
Member

fabpot commented Jul 4, 2021

@jnadaud Can you create a PR on symfony/recipes for this new notifier?

@nicolas-grekas
Copy link
Member

Tests fail also after this merge. Could you please have a look @jnadaud?

@OskarStark OskarStark mentioned this pull request Jul 5, 2021
Merged
@OskarStark
Copy link
Contributor

I am on it 👍

#41985

derrabus added a commit that referenced this pull request Jul 5, 2021
@derrabus
bug #41985 Fix: Mailjet bridge tests (OskarStark)
b19200c 
This PR was merged into the 5.4 branch.

Discussion
----------

Fix: Mailjet bridge tests

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | Follows #41705
| License       | MIT
| Doc PR        | -

Fix the build

Commits
-------

690e4ed Fix: Mailjet bridge tests
This was referenced Nov 5, 2021
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OskarStark OskarStark OskarStark approved these changes

@94noni 94noni 94noni left review comments

@fabpot fabpot fabpot approved these changes

Assignees
No one assigned
Labels
Feature Notifier Status: Reviewed
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.
6 participants
@jnadaud @carsonbot @OskarStark @fabpot @nicolas-grekas @94noni