Skip to content

[Notifier] Add Esendex bridge #36573

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
fabpot merged 1 commit into from
Aug 25, 2020
Merged

[Notifier] Add Esendex bridge #36573

fabpot merged 1 commit into from
Aug 25, 2020

Conversation

@odolbeau
Copy link
Contributor

@odolbeau odolbeau commented Apr 24, 2020

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
License MIT

Add Esendex notifier bridge.

@nicolas-grekas nicolas-grekas added this to the next milestone Apr 26, 2020
@nicolas-grekas nicolas-grekas changed the title Add Esendex Notifier bridge [Notifier] Add Esendex bridge Apr 26, 2020
@odolbeau odolbeau force-pushed the add-esendex-notifier-bridge branch from 8ff5d52 to e70e216 Compare April 28, 2020 21:39
fabpot
fabpot requested changes Aug 11, 2020
View reviewed changes
Copy link
Member

@fabpot fabpot left a comment

Choose a reason for hiding this comment

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

LGTM.
Can you also create a PR on symfony/recipes?

@odolbeau odolbeau force-pushed the add-esendex-notifier-bridge branch 2 times, most recently from 5fcb1b8 to 4eea7ae Compare August 25, 2020 13:16
@odolbeau odolbeau mentioned this pull request Aug 25, 2020
Merged
@odolbeau
Copy link
Contributor Author

odolbeau commented Aug 25, 2020 •
edited
Loading

PR updated according to previous reviews (thanks!) + improve error handling + added a few tests.

@odolbeau odolbeau requested a review from fabpot August 25, 2020 13:22
fabpot
fabpot requested changes Aug 25, 2020
View reviewed changes
Copy link
Member

@fabpot fabpot left a comment

Choose a reason for hiding this comment

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

Tests do not pass.

@odolbeau odolbeau force-pushed the add-esendex-notifier-bridge branch 3 times, most recently from 21010ff to 26637ea Compare August 25, 2020 14:08
@odolbeau odolbeau force-pushed the add-esendex-notifier-bridge branch 2 times, most recently from ba54492 to 544efe5 Compare August 25, 2020 14:36
@fabpot
Copy link
Member

fabpot commented Aug 25, 2020

@odolbeau The tests are still broken when low deps are used.

@odolbeau
Copy link
Contributor Author

odolbeau commented Aug 25, 2020

@fabpot This fix should do the job: a1b612d

@odolbeau odolbeau requested a review from fabpot August 25, 2020 16:24
@fabpot fabpot force-pushed the add-esendex-notifier-bridge branch from a1b612d to 34fc8c3 Compare August 25, 2020 19:06
@fabpot
Copy link
Member

fabpot commented Aug 25, 2020

Thank you @odolbeau.

@fabpot fabpot mentioned this pull request Aug 25, 2020
Closed
@fabpot fabpot merged commit bd26785 into symfony:master Aug 25, 2020
@odolbeau odolbeau deleted the add-esendex-notifier-bridge branch August 26, 2020 08:36
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020
@fabpot fabpot mentioned this pull request Oct 5, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fabpot fabpot fabpot approved these changes

+1 more reviewer

@noniagriconomie noniagriconomie noniagriconomie left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Feature Notifier Status: Reviewed

Projects

None yet

Milestone

5.2

Development

Successfully merging this pull request may close these issues.

5 participants

@odolbeau @fabpot @noniagriconomie @nicolas-grekas @carsonbot