NOISSUE - Add SMPP notifier #1464

blokovi · 2021-09-20T09:55:53Z

Signed-off-by: Ivan Milosevic iva@blokovi.com

What does this do?

Adds an implementation of SMPP Notifier (capability of sending SMS notifications).

Signed-off-by: Ivan Milosevic <iva@blokovi.com>

remove env file Signed-off-by: Ivan Milosevic <iva@blokovi.com>

codecov-commenter · 2021-09-20T10:01:40Z

Codecov Report

Merging #1464 (092ea0a) into master (f4312ae) will decrease coverage by 0.01%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##           master    #1464      +/-   ##
==========================================
- Coverage   70.92%   70.90%   -0.02%     
==========================================
  Files         123      123              
  Lines        9564     9565       +1     
==========================================
- Hits         6783     6782       -1     
- Misses       2254     2256       +2     
  Partials      527      527

Impacted Files	Coverage Δ
users/api/logging.go	`0.00% <0.00%> (ø)`
consumers/notifiers/service.go	`88.00% <100.00%> (+0.24%)`	⬆️
things/service.go	`72.02% <0.00%> (-1.20%)`	⬇️

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 f4312ae...092ea0a. Read the comment docs.

manuio · 2021-09-20T13:18:56Z

consumers/notifiers/service.go

@@ -28,7 +28,7 @@ var (
 	ErrSave = errors.New("failed to subscription")

 	// ErrNotFound indicates a non-existent entity request.
-	ErrNotFound = errors.New("non-existent entity")
+	ErrNotFound = errors.New("non-existent entityty")


Fix entityty

manuio · 2021-09-20T13:19:48Z

consumers/notifiers/service_test.go

@@ -29,7 +29,8 @@ func newService() notifiers.Service {
 	auth := mocks.NewAuth(map[string]string{exampleUser1: exampleUser1, exampleUser2: exampleUser2, invalidUser: invalidUser})
 	notifier := mocks.NewNotifier()
 	idp := uuid.NewMock()
-	return notifiers.New(auth, repo, idp, notifier)
+	from := ""


Mayeb better to don't use an empty string here?

I agree, simply use a string literal.

manuio · 2021-09-20T13:20:23Z

consumers/notifiers/api/endpoint_test.go

@@ -68,7 +68,8 @@ func newService(tokens map[string]string) notifiers.Service {
 	repo := mocks.NewRepo(make(map[string]notifiers.Subscription))
 	idp := uuid.NewMock()
 	notif := mocks.NewNotifier()
-	return notifiers.New(auth, repo, idp, notif)
+	from := ""


Mayeb better to don't use an empty string here?

dborovcanin · 2021-09-20T14:09:38Z

consumers/notifiers/service_test.go

@@ -29,7 +29,8 @@ func newService() notifiers.Service {
 	auth := mocks.NewAuth(map[string]string{exampleUser1: exampleUser1, exampleUser2: exampleUser2, invalidUser: invalidUser})
 	notifier := mocks.NewNotifier()
 	idp := uuid.NewMock()
-	return notifiers.New(auth, repo, idp, notifier)
+	from := ""


I agree, simply use a string literal.

dborovcanin · 2021-09-20T14:10:15Z

consumers/notifiers/smpp/README.md

+| MF_SMTP_NOTIFY_LOG_LEVEL          | Log level for SMT Notifier (debug, info, warn, error)                   | error                 |
+| MF_SMTP_NOTIFY_DB_HOST            | Database host address                                                   | localhost             |
+| MF_SMTP_NOTIFY_DB_PORT            | Database host port                                                      | 5432                  |
+| MF_SMTP_NOTIFY_DB_USER            | Database user                                                           | mainflux              |
+| MF_SMTP_NOTIFY_DB_PASS            | Database password                                                       | mainflux              |
+| MF_SMTP_NOTIFY_DB                 | Name of the database used by the service                                | subscriptions         |
+| MF_SMTP_NOTIFY_WRITER_CONFIG_PATH | Database connection SSL mode (disable, require, verify-ca, verify-full) | disable               |
+| MF_SMTP_NOTIFY_DB_SSL_MODE        | Path to the PEM encoded certificate file                                |                       |
+| MF_SMTP_NOTIFY_DB_SSL_CERT        | Path to the PEM encoded key file                                        |                       |
+| MF_SMTP_NOTIFY_DB_SSL_KEY         | Path to the PEM encoded root certificate file                           |                       |
+| MF_SMTP_NOTIFY_DB_SSL_ROOT_CERT   | Users service HTTP port                                                 | 8180                  |
+| MF_SMTP_NOTIFY_HTTP_PORT          | Path to server certificate in pem format                                |                       |
+| MF_SMTP_NOTIFY_SERVER_CERT        | Path to server cert in pem format                                       |                       |
+| MF_SMTP_NOTIFY_SERVER_KEY         | Path to server key in pem format                                        |                       |


dborovcanin · 2021-09-20T14:10:50Z

consumers/notifiers/smpp/README.md

+The Subscription service using SMTP Notifier is configured using the environment variables presented in the
+following table. Note that any unset variables will be replaced with their
+default values.
+


Please fix README.

dborovcanin · 2021-09-20T14:11:20Z

consumers/notifiers/smpp/notifier.go

+	fmt.Println(to)
+	fmt.Println(len(to))


dborovcanin · 2021-09-20T14:13:12Z

docker/addons/smpp-notifier/docker-compose.yml

+      MF_NATS_URL: ${MF_NATS_URL}
+      MF_JAEGER_URL: ${MF_JAEGER_URL}
+      MF_AUTH_GRPC_URL: ${MF_AUTH_GRPC_URL}
+      MF_AUTH_GRPC_TIMEOUT: ${MF_AUTH_GRPC_TIMEOUT}


You can group SMPP-related env vars by putting these on top of the environment section.

Signed-off-by: Ivan Milosevic <iva@blokovi.com>

dborovcanin · 2021-09-21T14:01:48Z

consumers/notifiers/smpp/notifier.go

+	if err != nil {
+		return err
+	}
+	fmt.Println("Message ID:", sm.RespID())


Remove logs.

Signed-off-by: Ivan Milosevic <iva@blokovi.com>

drasko · 2021-09-28T20:45:43Z

consumers/notifiers/smpp/notifier.go

+var _ notifiers.Notifier = (*notifier)(nil)
+
+type notifier struct {
+	t             *smpp.Transmitter


I would consider calling this struct field transmitter, and one below transformer. When it comes to struct fields, I find it easier to read the code - example on one of the lines below n.t.Submit() does not tell me much as n.transmitter.Submit().

@nmarcetic @dusanb94 opinions?

For me, both are fine. Transmitter is a bit more self-explanatory, t is shorter. When reading the code, the one will probably need to check what it is in any case.

Agree @drasko @dusanb94 , readability wins. I prefer to keep variable names short in scopes (like 1-3 letters), not struct fields where they should be obvious.

Signed-off-by: Ivan Milosevic <iva@blokovi.com>

drasko · 2021-10-06T11:44:55Z

consumers/notifiers/smpp/notifier.go

@@ -18,8 +18,8 @@ import (
 var _ notifiers.Notifier = (*notifier)(nil)

 type notifier struct {
-	t             *smpp.Transmitter
-	tr            transformers.Transformer
+	transmiter    *smpp.Transmitter


Typo: transmitter

Signed-off-by: Ivan Milosevic <iva@blokovi.com>

drasko

LGTM

* Add SMPP notifier Signed-off-by: Ivan Milosevic <iva@blokovi.com> * fix readme remove env file Signed-off-by: Ivan Milosevic <iva@blokovi.com> * resolve conversations Signed-off-by: Ivan Milosevic <iva@blokovi.com> * Remove debug log Signed-off-by: Ivan Milosevic <iva@blokovi.com> * Rename transmiter and transformer fields in struct Signed-off-by: Ivan Milosevic <iva@blokovi.com> * fix typo Signed-off-by: Ivan Milosevic <iva@blokovi.com>

blokovi added 2 commits September 20, 2021 11:43

Add SMPP notifier

758e9c4

Signed-off-by: Ivan Milosevic <iva@blokovi.com>

fix readme

06e9a9b

remove env file Signed-off-by: Ivan Milosevic <iva@blokovi.com>

blokovi requested a review from a team as a code owner September 20, 2021 09:55

manuio suggested changes Sep 20, 2021

View reviewed changes

dborovcanin requested changes Sep 20, 2021

View reviewed changes

resolve conversations

783dbb0

Signed-off-by: Ivan Milosevic <iva@blokovi.com>

dborovcanin approved these changes Sep 21, 2021

View reviewed changes

dborovcanin requested changes Sep 21, 2021

View reviewed changes

Remove debug log

3750e47

Signed-off-by: Ivan Milosevic <iva@blokovi.com>

dborovcanin previously approved these changes Sep 24, 2021

View reviewed changes

drasko requested changes Sep 28, 2021

View reviewed changes

Rename transmiter and transformer fields in struct

9b31fb9

Signed-off-by: Ivan Milosevic <iva@blokovi.com>

blokovi dismissed dborovcanin’s stale review via 9b31fb9 October 6, 2021 11:20

drasko requested changes Oct 6, 2021

View reviewed changes

fix typo

092ea0a

Signed-off-by: Ivan Milosevic <iva@blokovi.com>

drasko approved these changes Oct 6, 2021

View reviewed changes

drasko merged commit 66d3da0 into absmach:master Oct 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NOISSUE - Add SMPP notifier #1464

NOISSUE - Add SMPP notifier #1464

blokovi commented Sep 20, 2021

codecov-commenter commented Sep 20, 2021 •

edited

Loading

manuio Sep 20, 2021

manuio Sep 20, 2021

dborovcanin Sep 20, 2021

manuio Sep 20, 2021 •

edited

Loading

dborovcanin Sep 20, 2021

dborovcanin Sep 20, 2021

dborovcanin Sep 20, 2021

dborovcanin Sep 20, 2021

dborovcanin Sep 20, 2021

dborovcanin Sep 21, 2021

drasko Sep 28, 2021

dborovcanin Sep 29, 2021

nmarcetic Sep 30, 2021

drasko Oct 6, 2021

drasko left a comment

NOISSUE - Add SMPP notifier #1464

NOISSUE - Add SMPP notifier #1464

Conversation

blokovi commented Sep 20, 2021

What does this do?

codecov-commenter commented Sep 20, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manuio Sep 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drasko left a comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 20, 2021 •

edited

Loading

manuio Sep 20, 2021 •

edited

Loading