feat(service): Adds Mattermost service

[shivuslr41]

Description

Adds a notifier service for Mattermost

Motivation and Context

• Mattermost is an opensource alternative to slack. Mattermost has great community, supports various of tools integration, and also most of the features are FREE!
• Small teams can get great benefit from this messaging/collaboration service.

This addresses the issue #422
Also PR -> #424

How Has This Been Tested?

I tested this in 2 steps:

• Basic unit tests using mocks.
• Manual testing has been performed as well.

Screenshots / Output (if appropriate):

Messages from admin and non-admin users.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation (no code change)
• Refactor (refactoring production code)
• Other

Checklist:

• My code follows the code style of this project.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

[EthanEFung]

Hi @shivuslr41 , thanks so much for you patience and again this is a high quality PR. At first glance the usage of the http client is very nice and @nikoksr will be glad to see the go.sum is not even touched. I'm hoping to have a proper review for you this upcoming weekend.

[codecov-commenter]

Codecov Report

Patch coverage: 60.52% and project coverage change: -1.07 ⚠️

Comparison is base (67c5b79) 75.17% compared to head (5cc0ded) 74.10%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #516      +/-   ##
==========================================
- Coverage   75.17%   74.10%   -1.07%     
==========================================
  Files          42       44       +2     
  Lines        1458     1572     +114     
==========================================
+ Hits         1096     1165      +69     
- Misses        268      307      +39     
- Partials       94      100       +6     

Impacted Files	Coverage Δ
service/mattermost/mattermost.go	60.46% <60.46%> (ø)
service/mattermost/mock_http_client.go	60.71% <60.71%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[EthanEFung]

Another excellent PR @shivuslr41 . I like how well commented the code is, and the code is mostly straight forward. I have a few concerns regarding some of the logging, and other nit-picks but we're nearly ready to move forward with this PR.

[shivuslr41]

Hey @EthanEFung, Thank you very much for the time to review this!

I will work on the suggestions this weekend and yes! go.mod and go.sum are untouched, switching to a generic HTTP service was easy than I thought. Great work guys!

[EthanEFung]

Thanks for you patience @shivuslr41 . Work has picked up for me this past month and has been very busy. Anyways, as mentioned in the previous conversation, I think that adding the hooks to your message service to be a reasonable approach. That said, if the method is public in the mattermost service then we would like tests to be written for them. Let me know if you have any questions here and again, great work so far.

[shivuslr41]

Hi @EthanEFung, Thank you for your time again!

Added tests for PreSend and PostSend hooks now. The reason I left adding unit tests for these is they do not return anything.
But having tests for all the exposed methods makes sense.

[EthanEFung]

I have added mockClient.AssertExpectations(t) along with calling service.Send() method.

I'm assuming that you are suggesting checking if the httpClient (i.e., mockClient. not service.) pre-send and post-send hooks are called before and after send method call respectively. Not sure if we can do this since generated mockClient doesn't call (is not aware) these hooks in the generated mockClient.Send().

please suggest how this could be achieved if possible, that would be great for testing complete capabilities.

Ahh, I understand the predicament here. The mockClient doesn't call a pre or post hook so we can't make hook assertions based on calling .Send .

My feedback regarding the hook tests is that we want to assert something about the hooks. The current hook tests assert that the New function returns a service and calling service.Send it doesn't result in an error. We've already made these assertions in the other tests.

I propose to make things easier for you. Instead of the current hook tests implementations, rewrite the hook tests to assert that the mockClient hooks are called when calling the service hooks.

I feel these would be appropriate tests because the service hook implementations rely on the httpService hooks to work. While it might be a stretch to assert that a .Send call would result in the service hooks being called due to its dependency, it is realistic to expect that adding a service hook would create a httpClient hook.

Please let me know of any questions or comments before making code changes. You've already written so much, I want this PR to pass with flying colors and great quality.

[shivuslr41]

Got it! I found that assert.True() can be used to check if the expected methods are called. Maybe this is will be good enough for the hooks scenarios.

Let me know if any further suggestions/comments.

[EthanEFung]

Looks good @shivuslr41
@nikoksr this PR is ready to merge

[shivuslr41]

Hey @EthanEFung / @nikoksr, Hope you are doing well!

Just checking out of curiosity if anything is being planned for this PR before getting merged. I see codecov failures but couldn't figure out why, my guess is due to reduced test coverage % but not sure. Let me know if I can be of help in any way.

Again thank you for all the patience and review.