Skip to content

[Notifier] Add options to Microsoft Teams notifier #40738

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
Jun 23, 2021
Merged

[Notifier] Add options to Microsoft Teams notifier #40738

merged 1 commit into from
Jun 23, 2021

Conversation

OskarStark
Copy link
Contributor

@OskarStark OskarStark commented Apr 8, 2021 •
edited
Loading

Q A
Branch? 5.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets Follows #39007
License MIT
Doc PR symfony/symfony-docs#15288

After rework:

CleanShot 2021-04-15 at 09 40 45@2x

@OskarStark OskarStark self-assigned this Apr 8, 2021
Nyholm
Nyholm requested changes Apr 8, 2021
View reviewed changes
@jderusse jderusse added this to the 5.x milestone Apr 10, 2021
@OskarStark OskarStark force-pushed the notifier/microsot-teams-options branch from 14eea2a to 920961e Compare April 12, 2021 18:43
@OskarStark OskarStark force-pushed the notifier/microsot-teams-options branch 6 times, most recently from 4a18dd8 to a3e9511 Compare April 13, 2021 11:09
@OskarStark OskarStark force-pushed the notifier/microsot-teams-options branch from dd69d4d to 38c7a69 Compare April 13, 2021 11:20
@OskarStark OskarStark force-pushed the notifier/microsot-teams-options branch from 38c7a69 to d22a391 Compare April 13, 2021 11:38
@OskarStark OskarStark mentioned this pull request Apr 13, 2021
Closed
@OskarStark OskarStark requested review from Nyholm and jderusse April 14, 2021 06:39
Nyholm
Nyholm approved these changes Apr 14, 2021
View reviewed changes
Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Thank you. This looks way better now.

I've not tested the code, but I trust that you have.

@OskarStark OskarStark force-pushed the notifier/microsot-teams-options branch 2 times, most recently from d89f4b5 to 767698e Compare April 14, 2021 08:26
@OskarStark OskarStark force-pushed the notifier/microsot-teams-options branch from 16e1cfa to 79b1e7c Compare April 26, 2021 09:03
@javiereguiluz javiereguiluz mentioned this pull request Apr 30, 2021
Merged
@OskarStark OskarStark force-pushed the notifier/microsot-teams-options branch 2 times, most recently from 95129e9 to d997961 Compare May 20, 2021 10:26
chalasr
chalasr requested changes May 20, 2021
View reviewed changes
Copy link
Member

@chalasr chalasr left a comment •
edited
Loading

Choose a reason for hiding this comment

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

It seems that some interfaces are not actually needed.
Interfaces make maintenance harder so it must be worth it. I strongly think those should be removed, we can reconsider later if a use case comes out.

@OskarStark OskarStark force-pushed the notifier/microsot-teams-options branch from d997961 to 3fc7247 Compare May 20, 2021 11:01
@OskarStark OskarStark requested a review from chalasr May 20, 2021 11:02
@OskarStark OskarStark force-pushed the notifier/microsot-teams-options branch 2 times, most recently from 57c793d to 0039b9c Compare May 28, 2021 14:00
@OskarStark OskarStark force-pushed the notifier/microsot-teams-options branch 2 times, most recently from 4d275c8 to d86f7b4 Compare June 9, 2021 19:00
@OskarStark OskarStark requested a review from derrabus June 9, 2021 19:01
jderusse
jderusse reviewed Jun 9, 2021
View reviewed changes
Copy link
Member

@jderusse jderusse left a comment

Choose a reason for hiding this comment

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

I'm not a big fan of the protected options and everybody writing into it.
I wonder if the property shouldn't be private, and each class is responsible for its own properties. Then change the toArray method for something like

public function toArray(): array
{
  return parent::toArray() + $this->options + ['@action' => 'FooBar'];
}

@OskarStark
Copy link
Contributor Author

I'm not a big fan of the protected options and everybody writing into it.
I wonder if the property shouldn't be private, and each class is responsible for its own properties. Then change the toArray method for something like

@jderusse makes sense, did that 👍

@OskarStark OskarStark requested a review from jderusse June 17, 2021 09:09
@OskarStark OskarStark force-pushed the notifier/microsot-teams-options branch from 96d3019 to e33faae Compare June 18, 2021 06:24
@fabpot fabpot force-pushed the notifier/microsot-teams-options branch from e33faae to d039ce7 Compare June 23, 2021 14:22
@fabpot
Copy link
Member

fabpot commented Jun 23, 2021

Thank you @OskarStark.

@fabpot fabpot merged commit fa120c0 into symfony:5.4 Jun 23, 2021
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Jul 27, 2021
@javiereguiluz
minor #15288 [Notifier] Documentation for Microsoft Teams Options (Ka…
00f3420 
…milKubicki)

This PR was squashed before being merged into the 5.4 branch.

Discussion
----------

[Notifier] Documentation for Microsoft Teams Options

Docs for symfony/symfony#40738

Replaces #15232

Commits
-------

3c98ba8 [Notifier] Documentation for Microsoft Teams Options
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

@Nyholm Nyholm Nyholm approved these changes

@KamilKubicki KamilKubicki KamilKubicki requested changes

@fabpot fabpot fabpot approved these changes

@chalasr chalasr Awaiting requested review from chalasr

@derrabus derrabus Awaiting requested review from derrabus

@jderusse jderusse Awaiting requested review from jderusse

Assignees

@OskarStark OskarStark

Labels
Feature Notifier Status: Reviewed
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.
8 participants
@OskarStark @Nyholm @KamilKubicki @fabpot @jderusse @chalasr @nicolas-grekas @carsonbot