Skip to content
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.

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

feat(mattermost): add support for icons #237

Merged
merged 6 commits into from May 21, 2022
Merged

feat(mattermost): add support for icons #237

merged 6 commits into from May 21, 2022

Conversation

JosephKav
Copy link
Contributor

@JosephKav JosephKav commented May 17, 2022

  • What your PR contributes
    Icon support on MatterMost:
./shoutrrr send -u 'mattermost://Argus@mattermost.example.io:443/xxxxxxxxxxx?icon_emoji=https%3A%2F%2Fraw.githubusercontent.com%2Fcontainrrr%2Fshoutrrr%2Fmain%2Fdocs%2Fshoutrrr-logotype.png' -m 'Icon Test'
./shoutrrr send -u 'mattermost://Argus@mattermost.example.io:443/xxxxxxxxxxx?icon_emoji=smiley' -m 'Icon Test'

image

  • Which issues it solves
    Moving my Argus project to use Shoutrrr. Really want to keep icon support!

  • Tests that verify the code your contributing
    I tried adding tests (and most of this code is copied from the Slack service), but couldn't get them to pass. Every other service that has query params seems to include the title param, but that's no supported on MatterMost, so I'm not sure what to do w.r.t this error. Help fixing this would be appreciated!

• Failure [0.000 seconds]
services
/path/to/shoutrrr/pkg/services/services_test.go:48
  when passed the a title param
  /path/to/shoutrrr/pkg/services/services_test.go:57
    should not throw an error for mattermost [It]
    /path/to/shoutrrr/pkg/services/services_test.go:71

    Unexpected error:
        <*errors.errorString | 0xc00030e1b0>: {
            s: "title is not a valid config key [icon icon_emoji icon_url]",
        }
        title is not a valid config key [icon icon_emoji icon_url]
    occurred

    /path/to/shoutrrr/pkg/services/services_test.go:99
  • Updates to the documentation
    Done? Let me know if you'd like any more, or changes to them. (Hoping the 'The services does not support any query/param props' would go away with this current commit?)

@piksel
Copy link
Member

piksel commented May 17, 2022

Every other service that has query params seems to include the title param, but that's no supported on MatterMost.

Just add it anyway. The point is that you should be able to pass title to all services without checking for support. If they don't support it, they are free to just ignore it.
So either put it in the config with key:"title", or manually explicitly ignore it.

Co-authored-by: nils måsén <nils@piksel.se>
@JosephKav
Copy link
Contributor Author

JosephKav commented May 17, 2022

Thanks for such a quick response and even giving me the commit suggestion for the fix! 😄
(go test ./... locally and all passed)

@codecov
Copy link

codecov bot commented May 17, 2022

Codecov Report

Merging #237 (4a9e231) into main (5bca245) will increase coverage by 0.21%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #237      +/-   ##
==========================================
+ Coverage   76.85%   77.07%   +0.21%     
==========================================
  Files          95       95              
  Lines        2826     2844      +18     
==========================================
+ Hits         2172     2192      +20     
+ Misses        474      473       -1     
+ Partials      180      179       -1     
Impacted Files Coverage Δ
pkg/services/mattermost/mattermost.go 75.00% <100.00%> (+15.00%) ⬆️
pkg/services/mattermost/mattermost_config.go 93.93% <100.00%> (+2.27%) ⬆️
pkg/services/mattermost/mattermost_json.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5bca245...4a9e231. Read the comment docs.

@JosephKav
Copy link
Contributor Author

Any tips to get the codecov higher?
The two that show up on the files page are return err being missed after the following
if err := resolver.Set(key, vals[0]); err != nil {
or
if err := service.pkr.UpdateConfigFromParams(config, params); err != nil {
Any tests that'd get into these?

@JosephKav JosephKav requested a review from piksel May 17, 2022 18:35
@JosephKav
Copy link
Contributor Author

Could I get some assistance on increasing the CodeCov please?

@piksel
Copy link
Member

piksel commented May 20, 2022

You can use the generic service compliance tests:

Describe("the basic service API", func() {
Describe("the service config", func() {
It("should implement basic service config API methods correctly", func() {
testutils.TestConfigGetInvalidQueryValue(&Config{})
testutils.TestConfigSetInvalidQueryValue(&Config{}, "bark://:mock-device@host/?foo=bar")
testutils.TestConfigSetDefaultValues(&Config{})
testutils.TestConfigGetEnumsCount(&Config{}, 0)
testutils.TestConfigGetFieldsCount(&Config{}, 9)
})
})
Describe("the service instance", func() {
BeforeEach(func() {
httpmock.Activate()
})
AfterEach(func() {
httpmock.DeactivateAndReset()
})
It("should implement basic service API methods correctly", func() {
serviceURL := testutils.URLMust("bark://:devicekey@hostname")
Expect(service.Initialize(serviceURL, logger)).To(Succeed())
testutils.TestServiceSetInvalidParamValue(service, "foo", "bar")
})
})
})

@JosephKav
Copy link
Contributor Author

JosephKav commented May 20, 2022

Hold off on approving this/starting the workflows. Adding more tests and fixing that latest one
All should be sorted now

Copy link
Member

@piksel piksel left a comment

Choose a reason for hiding this comment

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

Great work! I changed the scheme in the test URL, it doesn't matter for the test, but it's confusing to have the name of another service in the URL.

@piksel piksel merged commit 6dbcb12 into containrrr:main May 21, 2022
@piksel
Copy link
Member

piksel commented May 24, 2022

@all-contributors add @JosephKav for code

@allcontributors
Copy link
Contributor

@piksel

I've put up a pull request to add @JosephKav! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants