Feature/UPPSF-981 Add Annotations Notifications #83

ivan-p-nikolov · 2020-01-07T13:23:28Z

Remove profiler endpoints.
Fix bug preventing service to shutdown gracefully.
Add support for listening for multiple topics.
Add new subscription type Annotations.

dispatch/notifications.go

MiroslavGatsanoga · 2020-01-08T12:15:58Z

consumer/mapper.go

+		APIURL:           n.APIBaseURL + "/" + n.Resource + "/" + event.ContentID,
+		PublishReference: transactionID,
+		ContentType:      dispatch.AnnotationContentType,
+		LastModified:     time.Now().Format(time.RFC3339),


The way this is being set, it looks very similar to the NotificationDate field. Can't we propagate the last modified value for the annotations in similar manner as for the contents?

For content we get LastModified from the message body. But as far as I can see we don't have such information in the annotation message.

In the native store there is a lastModified field for the annotations, the question is do we want to propagate it through the pipeline so that we get it here. I am not sure if we are using similar approach for content so it might be a good thing to check.

consumer/metadata_test.go

consumer/publish_event.go

helm/notifications-push/app-configs/notifications-push_delivery.yaml

resources/push.go

epavlova · 2020-01-09T09:52:44Z

consumer/metadata.go

+func (h *metadataQueueHandler) HandleMessage(queueMsg kafka.FTMessage) error {
+	msg := NotificationQueueMessage{queueMsg}
+	tid := msg.TransactionID()
+	monitoringLogger := log.WithMonitoringEvent("NotificationsPush", tid, "annotations update")


I know that it's a bit misleading but we shouldn't have logging with WithMonitoringEvent event here. The initial idea that our colleagues had was to log with WithMonitoringEvent a few events that track significant events when a content is being published - saved to native store, saved to document store, notification sent. Logging in this way here would mess with that intention as this event here have nothing to do with content publishing. Although our colleagues didn't implement the monitoring based on these events, it still may be confusing and will go to the monitoring Kinesis stream with no reason.

epavlova · 2020-01-09T10:05:07Z

test/mocks/mocks.go

@@ -14,36 +14,65 @@ import (
 // MockDispatcher is a mock of a dispatcher that can be reused for testing
 type MockDispatcher struct {
 	mock.Mock
+	StartFunc       func()


One recommendation only - the implemented method for mocking in this PR is something that I would always choose at least as starting writing a mock object. However here we already use testify mock.Mock which actually can do the same that we manually do with the private methods. So ideally we should have either mock.Mock or no library for mocking and manually write the private methods and so on. I see that in the new and in the old tests we are using MockDispather in very different ways. So the recommendation would be - if there are already a lot of tests using testify mock.Mock there is almost no gain in implementing the manual approach for writing mocks.

Apparently I was only using one of the mocked methods. So I rewrote the tests to use testify mock.

coveralls · 2020-01-09T14:42:55Z

Coverage decreased (-0.6%) to 65.473% when pulling 79a7766 on feature/UPPSF-981-add-annotations-notifications into 6435d06 on master.

MiroslavGatsanoga · 2020-01-10T11:43:16Z

consumer/mapper.go

+		APIURL:           n.APIBaseURL + "/" + n.Resource + "/" + event.ContentID,
+		PublishReference: transactionID,
+		ContentType:      dispatch.AnnotationContentType,
+		LastModified:     time.Now().Format(time.RFC3339),


In the native store there is a lastModified field for the annotations, the question is do we want to propagate it through the pipeline so that we get it here. I am not sure if we are using similar approach for content so it might be a good thing to check.

consumer/publish_event.go

Add configuration for whitelisting annotation origins Change notification schema - `Standout` is not mandatory now Change dispatcher mock to be able to override methods.

…mplify testing.

MiroslavGatsanoga

All looks good, just a couple of things that we make sure we have:

We should add to the README some information about the new type of notifications and how to subscribe to them. Also we should point out that content type All applies only for content subscriptions and the newly added annotation ones.
We should be able to subscribe through the public endpoints, lets check that we are able to do that when we deploy to upper environments. We should check the API endpoint as well as both delivery clusters.

Other than that I think the PR is ready to be merged.

Feature/UPPSF-981 Add Annotations Notifications

ivan-p-nikolov added 4 commits December 11, 2019 11:45

Remove profiler endpoints

5b41e90

Fix bug preventing service to shutdown gracefully.

dbf9833

Add support for listening for multiple topics

f9e836f

Separate message handlers to content and metadata

1a632f6

ivan-p-nikolov requested a review from a team January 7, 2020 13:23

epavlova self-requested a review January 7, 2020 13:26

MiroslavGatsanoga reviewed Jan 8, 2020

View reviewed changes

epavlova requested changes Jan 9, 2020

View reviewed changes

ivan-p-nikolov force-pushed the feature/UPPSF-981-add-annotations-notifications branch from c1e8e12 to 0bd3e0c Compare January 9, 2020 14:40

ivan-p-nikolov force-pushed the feature/UPPSF-981-add-annotations-notifications branch from 0bd3e0c to ea299b2 Compare January 9, 2020 14:47

ivan-p-nikolov requested review from MiroslavGatsanoga and epavlova January 9, 2020 15:08

MiroslavGatsanoga reviewed Jan 10, 2020

View reviewed changes

ivan-p-nikolov force-pushed the feature/UPPSF-981-add-annotations-notifications branch from ea299b2 to abd9a46 Compare January 13, 2020 12:24

MiroslavGatsanoga self-requested a review January 13, 2020 12:32

ivan-p-nikolov force-pushed the feature/UPPSF-981-add-annotations-notifications branch from abd9a46 to f910e0d Compare January 14, 2020 13:05

ivan-p-nikolov added 4 commits January 15, 2020 10:21

Start handling annotation change events

b0b05b3

Add configuration for whitelisting annotation origins Change notification schema - `Standout` is not mandatory now Change dispatcher mock to be able to override methods.

Add new subscription type Annotations

af83784

Rename the ambiguous contentType to subscriptionType

fe210b7

Extract LastModified workaround for annotations as an interface to si…

9496b06

…mplify testing.

ivan-p-nikolov force-pushed the feature/UPPSF-981-add-annotations-notifications branch from f910e0d to 9496b06 Compare January 15, 2020 08:24

MiroslavGatsanoga reviewed Jan 15, 2020

View reviewed changes

Add documentation for Annotations subsctiption

79a7766

ivan-p-nikolov requested a review from MiroslavGatsanoga January 16, 2020 09:16

MiroslavGatsanoga approved these changes Jan 16, 2020

View reviewed changes

epavlova approved these changes Jan 17, 2020

View reviewed changes

ivan-p-nikolov merged commit 5ba7d0f into master Jan 17, 2020

ivan-p-nikolov mentioned this pull request Jan 22, 2020

Revert "Feature/UPPSF-981 Add Annotations Notifications" #84

Merged

ivan-p-nikolov added a commit that referenced this pull request Jan 23, 2020

Introduce back features from PR #83

b05abe7

Feature/UPPSF-981 Add Annotations Notifications

ivan-p-nikolov deleted the feature/UPPSF-981-add-annotations-notifications branch February 21, 2020 15:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/UPPSF-981 Add Annotations Notifications #83

Feature/UPPSF-981 Add Annotations Notifications #83

ivan-p-nikolov commented Jan 7, 2020

MiroslavGatsanoga Jan 8, 2020

ivan-p-nikolov Jan 9, 2020

MiroslavGatsanoga Jan 10, 2020

epavlova Jan 9, 2020

epavlova Jan 9, 2020

ivan-p-nikolov Jan 9, 2020

coveralls commented Jan 9, 2020 •

edited

Loading

MiroslavGatsanoga Jan 10, 2020

MiroslavGatsanoga left a comment

Feature/UPPSF-981 Add Annotations Notifications #83

Feature/UPPSF-981 Add Annotations Notifications #83

Conversation

ivan-p-nikolov commented Jan 7, 2020

MiroslavGatsanoga Jan 8, 2020

Choose a reason for hiding this comment

ivan-p-nikolov Jan 9, 2020

Choose a reason for hiding this comment

MiroslavGatsanoga Jan 10, 2020

Choose a reason for hiding this comment

epavlova Jan 9, 2020

Choose a reason for hiding this comment

epavlova Jan 9, 2020

Choose a reason for hiding this comment

ivan-p-nikolov Jan 9, 2020

Choose a reason for hiding this comment

coveralls commented Jan 9, 2020 • edited Loading

MiroslavGatsanoga Jan 10, 2020

Choose a reason for hiding this comment

MiroslavGatsanoga left a comment

Choose a reason for hiding this comment

coveralls commented Jan 9, 2020 •

edited

Loading