feat(service): Add Ntfy #469

pushkar803 · 2022-11-17T15:09:22Z

Description

Motivation and Context

There was feature request created in issue to add Ntfy service #461

How Has This Been Tested?

Test program included in readme.md file

Screenshots / Output (if appropriate):

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.

svaloumas

Hello @pushkar803 !

First of all, thank you very much for your time and contribution! We really appreciate it!

Your PR needs some improvements to comply with the standards that we're trying to set for the project. We would be more than happy if you'd resolve those issues at your convenience!

Some basic guidelines for a new service implementation that we're trying to adhere to are:

The actual service implementation
A usage guide for the new service (you provided both of them! 🙏 )
Mocks generated by the use of mockery in order to unit-test your implementation

You can find hints on how to do so in our CONTRIBUTING.md

Tests at least for the exported functions of the new service (we're striving to increase our test coverage)
A doc.go file including instructions on how to use the service programmatically, please find a reference here

Also, I left some comments regarding your implementation. If you have any questions or might need any guidance, etc. please always feel free to shoot it and I and/or @nikoksr would be happy to provide help.

service/ntfy/ntfy.go

…e error

nikoksr · 2022-12-09T11:00:29Z

service/ntfy/ntfy.go

+}
+
+// DefaultServerURL is the default server to use for the Ntfy service.
+const DefaultServerURL = "https://ntfy.sh"


I don't see a need for this to be exported. We can keep the usage experience a little bit cleaner by making it non-exported.

Suggested change

const DefaultServerURL = "https://ntfy.sh"

const defaultServerURL = "https://ntfy.sh"

nikoksr · 2022-12-09T11:15:39Z

service/ntfy/ntfy.go

+// NewWithServers returns a new instance of Ntfy service. You can use this service to send messages to Ntfy. You can
+// specify the servers to send the messages to. By default, the service will use the default server
+// (https://api.day.app/) if you don't specify any servers.
+func NewWithServers(serverURLs ...string) *Service {


@svaloumas @pushkar803 I'm torn about this one. On one hand I like the convenience this offers (we're doing the same in the notify package), but on the other hand I see two things I dislike.

No other service (to my knowledge) implements this type of function so far, which makes the library less predictable and intuitiv. So either, we adapt this pattern and extend all or most other services with the same function or we remove it from here.

The inconsistent naming. We have a AddReceivers and NewWithServers, the latter calls AddReceivers internally. I think, in case we stick with it, this should be renamed to NewWithReceivers to stay consistent and intuitiv.

A call to NewWithServers without is functionally identical to just calling New with an empty parameter list. So maybe, we can resolve my issue above (no. 2) by dropping NewWithServers entirely and extend New with a variadic receivers list. However, this again makes the package not follow our current behavioral standards, which makes it less predictable. Maybe it's better if we stay explicit with two seperate constructor functions as it currently is the case. Feedback on this please!

nikoksr · 2022-12-09T11:16:42Z

service/ntfy/ntfy.go

+	if len(serverURLs) == 0 {
+		serverURLs = append(serverURLs, DefaultServerURL)
+	}


Length check is obsolete. An empty list will not alter the existsing serverURLs anyway.

Suggested change

if len(serverURLs) == 0 {

serverURLs = append(serverURLs, DefaultServerURL)

}

serverURLs = append(serverURLs, DefaultServerURL)

nikoksr · 2022-12-09T11:18:25Z

service/ntfy/ntfy.go

+	isJson := json.Valid(bodyByte)
+	if !isJson {
+		err := errors.New("Invalid JSON Body")
+		return err
+	}


Potential simplification.

Suggested change

isJson := json.Valid(bodyByte)

if !isJson {

err := errors.New("Invalid JSON Body")

return err

}

if !json.Valid(bodyByte) {

return errors.New("Invalid JSON Body")

}

nikoksr · 2022-12-09T11:18:41Z

service/ntfy/ntfy.go

+		err := errors.Wrap(err, "Invalid PostData structure")
+		return err


Suggested change

err := errors.Wrap(err, "Invalid PostData structure")

return err

return errors.Wrap(err, "Invalid PostData structure")

nikoksr · 2022-12-09T11:23:55Z

service/ntfy/ntfy_test.go

+func TestNtfy_send(t *testing.T) {
+	t.Parallel()
+
+	assert := require.New(t)
+
+	service := New()
+	assert.NotNil(service)
+
+	service.client = defaultHTTPClient()
+
+	ctx := context.Background()
+	serverURL := DefaultServerURL
+	topic := "pushkar"
+	content := `{
+		"message": "Disk space is low at 5.1 GB",
+		"title": "Low disk space alert",
+		"tags": ["warning","cd"],
+		"priority": 4,
+		"attach": "https://filesrv.lan/space.jpg",
+		"filename": "diskspace.jpg",
+		"click": "https://homecamera.lan/xasds1h2xsSsa/",
+		"actions": [{ "action": "view", "label": "Admin panel", "url": "https://filesrv.lan/admin" }]
+	}`
+
+	err := service.send(ctx, serverURL, topic, content)
+	assert.Nil(err)
+
+	err = service.send(ctx, "", topic, content)
+	assert.NotNil(err)
+
+	err = service.send(ctx, "xyz", topic, content)
+	assert.NotNil(err)
+
+	err = service.send(ctx, serverURL, topic, "sample_body")
+	assert.NotNil(err)
+
+	content = `{
+		"message": "Disk space is low at 5.1 GB",
+		"title": "Low disk space alert",
+		"priority": 4222,
+	}`
+
+	err = service.send(ctx, serverURL, topic, content)
+	assert.NotNil(err)
+
+}
+
+func TestNtfy_Send(t *testing.T) {
+	t.Parallel()
+
+	assert := require.New(t)
+
+	service := New()
+	assert.NotNil(service)
+
+	ctx := context.Background()
+	topic := "pushkar"
+	content := `{
+		"message": "Disk space is low at 5.1 GB",
+		"title": "Low disk space alert",
+		"tags": ["warning","cd"],
+		"priority": 4,
+		"attach": "https://filesrv.lan/space.jpg",
+		"filename": "diskspace.jpg",
+		"click": "https://homecamera.lan/xasds1h2xsSsa/",
+		"actions": [{ "action": "view", "label": "Admin panel", "url": "https://filesrv.lan/admin" }]
+	}`
+
+	err := service.Send(ctx, topic, content)
+	assert.Nil(err)
+
+	content = `{
+		"message": "Disk space is low at 5.1 GB",
+		"title": "Low disk space alert",
+		"priority": 4222,
+	}`
+
+	err = service.Send(ctx, topic, content)
+	assert.NotNil(err)
+}


If I'm not completely misunderstanding this, we're pinging the actual endpoint here. In that case, we can't let this go through. We're solely relying on mocked tests for all other services and should do the same here.

nikoksr · 2022-12-09T11:27:37Z

@pushkar803 we greatly appreciate your contribution! So excited about this.

Please take a look at the feedback. The crucial points to me are the tests for the send methods and the missing doc.go file. The latter should be easily addable. Just throw a look here for inspiration. You already provided a readme, which is awesome, and I think you can apply most of that to the doc.go file, too.

nikoksr · 2022-12-19T10:15:53Z

@pushkar803 anything I can help you with to push this forward? Please let me know if anything is unclear or you need help with something.

gedw99 · 2023-07-03T12:01:12Z

A few days ago Ntfy ( https://github.com/binwiederhier/ntfy ) with PWA web push is now in main head.
The A2HS now works too.

You can try it here: https://ntfy.sh/app

On Mobile or Desktop you can do Add to Home Screen and notifications do work. Its very easily release with a few small bugs still but will get better.

Dev is ticking along well so def worth trying to get this PR in i think.

nikoksr · 2023-07-13T19:04:19Z

@gedw99 I'll get working on this soon. Closing since it's dead.

ntfy service is added

159fd55

pull-request-size bot added the size/L label Nov 17, 2022

pushkar803 mentioned this pull request Nov 17, 2022

feat(service): Add Ntfy service #461

Open

svaloumas reviewed Nov 18, 2022

View reviewed changes

service/ntfy/ntfy.go Show resolved Hide resolved

service/ntfy/ntfy.go Outdated Show resolved Hide resolved

service/ntfy/ntfy.go Outdated Show resolved Hide resolved

service/ntfy/ntfy.go Outdated Show resolved Hide resolved

Pushkar Shetye added 2 commits November 27, 2022 09:42

ntfy service modified to work with HTTPClient and return ntfy respons…

0518549

…e error

Merge branch 'main' into feat/NtfyService

e0837c4

arihantthriwe approved these changes Nov 28, 2022

View reviewed changes

nikoksr requested changes Dec 9, 2022

View reviewed changes

nikoksr changed the title ~~Ntfy service added~~ feat(service): Add Ntfy Dec 19, 2022

nikoksr closed this Jul 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(service): Add Ntfy #469

feat(service): Add Ntfy #469

pushkar803 commented Nov 17, 2022

svaloumas left a comment •

edited

Loading

nikoksr Dec 9, 2022

nikoksr Dec 9, 2022

nikoksr Dec 9, 2022

nikoksr Dec 9, 2022

nikoksr Dec 9, 2022

nikoksr Dec 9, 2022

nikoksr commented Dec 9, 2022

nikoksr commented Dec 19, 2022

gedw99 commented Jul 3, 2023

nikoksr commented Jul 13, 2023

	const DefaultServerURL = "https://ntfy.sh"
	const defaultServerURL = "https://ntfy.sh"

		err := errors.Wrap(err, "Invalid PostData structure")
		return err

	err := errors.Wrap(err, "Invalid PostData structure")
	return err
	return errors.Wrap(err, "Invalid PostData structure")

feat(service): Add Ntfy #469

feat(service): Add Ntfy #469

Conversation

pushkar803 commented Nov 17, 2022

Description

Motivation and Context

How Has This Been Tested?

Screenshots / Output (if appropriate):

Types of changes

Checklist:

svaloumas left a comment • edited Loading

Choose a reason for hiding this comment

nikoksr Dec 9, 2022

Choose a reason for hiding this comment

nikoksr Dec 9, 2022

Choose a reason for hiding this comment

nikoksr Dec 9, 2022

Choose a reason for hiding this comment

nikoksr Dec 9, 2022

Choose a reason for hiding this comment

nikoksr Dec 9, 2022

Choose a reason for hiding this comment

nikoksr Dec 9, 2022

Choose a reason for hiding this comment

nikoksr commented Dec 9, 2022

nikoksr commented Dec 19, 2022

gedw99 commented Jul 3, 2023

nikoksr commented Jul 13, 2023

svaloumas left a comment •

edited

Loading