Skip to content

[Notifier] Add LinkedIn provider #37830

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
Aug 18, 2020
Merged

[Notifier] Add LinkedIn provider #37830

merged 1 commit into from
Aug 18, 2020

Conversation

ismail1432
Copy link
Contributor

@ismail1432 ismail1432 commented Aug 13, 2020 •
edited
Loading

Q A
Branch? master for features 5.2
Bug fix? no
New feature? yes/no
Deprecations? yes/no
Tickets Fix #34563
License MIT
Doc PR symfony/symfony-docs#...

Add the LinkedIn provider for the Notifier

Few months ago I created a bridge to send message to LinkedIn, following the discussion here I create the PR.

If you're curious I wrote an article where I use the bridge

[Edit] test are green missing improvement body construction and integration test with update changes + framework integration

Notification sent with this PR

YaFou
YaFou suggested changes Aug 13, 2020
View reviewed changes
Copy link
Contributor

@YaFou YaFou left a comment

Choose a reason for hiding this comment

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

Small review ;)

@ismail1432 ismail1432 force-pushed the notifier-add-linkedin-provider branch 2 times, most recently from af79138 to e5dfb1d Compare August 15, 2020 08:50
@ismail1432 ismail1432 changed the title [Notifier] WIP Add linkedin provider [Notifier] Add LinkedIn provider Aug 16, 2020
HaiYassin
HaiYassin reviewed Aug 17, 2020
View reviewed changes
Copy link

@HaiYassin HaiYassin left a comment

Choose a reason for hiding this comment

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

GG for this working

@ismail1432
Copy link
Contributor Author

failing test seems not related 🤔

fabpot
fabpot requested changes Aug 18, 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.

Minor style changes needed.

src/Symfony/Component/Notifier/Bridge/LinkedIn/LinkedInOptions.php Outdated
public static function fromNotification(Notification $notification): self
{
$options = new self();
$options->specificContent((new ShareContentShare($notification->getSubject())));
Copy link
Member

Choose a reason for hiding this comment

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

extra () can be removed (same 3 more times below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

src/Symfony/Component/Notifier/Bridge/LinkedIn/LinkedInTransportFactory.php Outdated
return (new LinkedInTransport($authToken, $accountId, $this->client, $this->dispatcher))
->setHost($host)
->setPort($port)
;
Copy link
Member

Choose a reason for hiding this comment

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

Can be on one line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

src/Symfony/Component/Notifier/Bridge/LinkedIn/README.md Outdated
@@ -0,0 +1,20 @@
LinkedIn Notifier
====================
Copy link
Member

Choose a reason for hiding this comment

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

extra =

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

src/Symfony/Component/Notifier/Bridge/LinkedIn/Share/ShareMediaShare.php Outdated
bool $landingPage = false,
string $landingPageTitle = null,
string $landingPageUrl = null
) {
Copy link
Member

Choose a reason for hiding this comment

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

should be on one line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@ismail1432 ismail1432 force-pushed the notifier-add-linkedin-provider branch from 011d958 to 0064cae Compare August 18, 2020 08:43
@ismail1432 ismail1432 requested a review from fabpot August 18, 2020 09:01
@fabpot
Copy link
Member

fabpot commented Aug 18, 2020

@ismail1432 Can you submit a PR on symfony/recipes?

@fabpot
Copy link
Member

fabpot commented Aug 18, 2020

Thank you @ismail1432.

@fabpot fabpot merged commit 14c9d05 into symfony:master Aug 18, 2020
@fabpot fabpot mentioned this pull request Aug 18, 2020
Closed
@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

@HaiYassin HaiYassin HaiYassin left review comments

@SHTIKOV SHTIKOV SHTIKOV left review comments

@YaFou YaFou YaFou requested changes

@fabpot fabpot fabpot approved these changes

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

Successfully merging this pull request may close these issues.

[Notifier] Add a LinkedInTransport
6 participants
@ismail1432 @fabpot @HaiYassin @SHTIKOV @YaFou @carsonbot