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.

Sign up for GitHub

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

Jump to bottom

Adding a new deployment for Lists push notifications #119

Merged
merged 5 commits into from
Oct 20, 2021
Merged

Adding a new deployment for Lists push notifications #119

merged 5 commits into from
Oct 20, 2021

Conversation

martin-stanchev
Copy link
Contributor

@martin-stanchev martin-stanchev commented Oct 6, 2021 •
edited
Loading

Description

What

Adding a new deployment for lists push notifications. The notifications are served at /lists/notifications-push and only allow List content type.

Why

Jira Ticket

Anything, in particular, you'd like to highlight to reviewers

Mention here sections of code which you would like reviewers to pay extra attention to .E.g

Would appreciate a second pair of eyes on the test
I am not quite sure how this bit works
Is there a better library for doing x

Scope and particulars of this PR (Please tick all that apply)

  • Tech hygiene (dependency updating & other tech debt)
  • Bug fix
  • Feature
  • Documentation
  • Breaking change
  • Minor change (e.g. fixing a typo, adding config)

This Pull Request follows the rules described in our Pull Requests Guide

@martin-stanchev martin-stanchev requested a review from a team as a code owner October 6, 2021 13:23
@martin-stanchev martin-stanchev force-pushed the feature/UPPSF-2731-lists-push-notifications branch 2 times, most recently from 6538015 to 9bfc894 Compare October 6, 2021 14:17
atanasdinov
atanasdinov suggested changes Oct 6, 2021
View reviewed changes
Copy link
Contributor

@atanasdinov atanasdinov left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The changes do work however we need to address some potential problems here.

Additionally, having in mind that lists are not "content" by definition we should also add tests for such notifications - unit, integration or both if applicable / necessary.

helm/notifications-push/app-configs/notifications-push-lists_eks_delivery.yaml Outdated Show resolved Hide resolved
helm/notifications-push/app-configs/notifications-push-lists_eks_delivery.yaml Outdated Show resolved Hide resolved
helm/notifications-push/app-configs/notifications-push-lists_eks_delivery.yaml Outdated Show resolved Hide resolved
helm/notifications-push/app-configs/notifications-push-lists_eks_delivery.yaml Outdated Show resolved Hide resolved
consumer/mapper.go Show resolved Hide resolved
resources/push.go Outdated Show resolved Hide resolved
helm/notifications-push/app-configs/notifications-push-lists_eks_delivery.yaml Outdated Show resolved Hide resolved
helm/notifications-push/app-configs/notifications-push-lists_eks_delivery.yaml Outdated Show resolved Hide resolved
@atanasdinov atanasdinov requested a review from a team October 6, 2021 14:26
@coveralls
Copy link

coveralls commented Oct 7, 2021 •
edited
Loading

Coverage Status

Coverage decreased (-0.9%) to 65.358% when pulling 21ecde3 on feature/UPPSF-2731-lists-push-notifications into c1b7c86 on master.

@martin-stanchev martin-stanchev force-pushed the feature/UPPSF-2731-lists-push-notifications branch 3 times, most recently from d6f25a2 to d71c4c6 Compare October 11, 2021 15:21
@ivanruski ivanruski self-requested a review October 12, 2021 07:15
ivanruski
ivanruski previously approved these changes Oct 12, 2021
View reviewed changes
@martin-stanchev martin-stanchev force-pushed the feature/UPPSF-2731-lists-push-notifications branch from 80d7d46 to 0a0bb93 Compare October 12, 2021 13:27
atanasdinov
atanasdinov reviewed Oct 12, 2021
View reviewed changes
Copy link
Contributor

@atanasdinov atanasdinov left a comment

Choose a reason for hiding this comment

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

The changes are great overall. I left some suggestions you could take a look at and discuss.

Please also consider adding test cases at least for subscription (in resources/push_test.go) and mapping (in consumer/mapper_test.go) .

resources/push.go Outdated Show resolved Hide resolved
push_service.go Outdated Show resolved Hide resolved
consumer/mapper.go Outdated Show resolved Hide resolved
dispatch/notifications.go Outdated Show resolved Hide resolved
helm/notifications-push/app-configs/notifications-push-lists_eks_delivery.yaml Outdated Show resolved Hide resolved
app.go Show resolved Hide resolved
README.md
@@ -92,6 +108,13 @@ E.g.
You can be subscribed for multiple types:
```curl -i --header "x-api-key: «api_key»" https://api.ft.com/content/notifications-push?type=All&type=LiveBlogPost&type=LiveBlogPackage```

###For lists:

```curl -i --header "x-api-key: «api_key»" https://api.ft.com/lists/notifications-push```
Copy link
Contributor

Choose a reason for hiding this comment

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

This raises a question which probably only API Gateway could answer but we might need to address it. Currently all of the API keys with Push API - Content policy will work for the /lists endpoint however there's a policy called Push API - Lists which will also work. Do we need API Gateway to fix this on their side so we can reflect it in our documentation as well?

@atanasdinov atanasdinov requested a review from a team October 12, 2021 16:03
@martin-stanchev martin-stanchev force-pushed the feature/UPPSF-2731-lists-push-notifications branch from f302a38 to bf82c39 Compare October 13, 2021 10:52
ivanruski
ivanruski previously approved these changes Oct 13, 2021
View reviewed changes
@martin-stanchev martin-stanchev force-pushed the feature/UPPSF-2731-lists-push-notifications branch from 376cce2 to dec42e3 Compare October 13, 2021 14:00
@martin-stanchev martin-stanchev force-pushed the feature/UPPSF-2731-lists-push-notifications branch 4 times, most recently from 109ea53 to 6750e29 Compare October 14, 2021 07:29
atanasdinov
atanasdinov previously approved these changes Oct 14, 2021
View reviewed changes
@atanasdinov atanasdinov requested a review from a team October 14, 2021 08:02
dtvalk-ov
dtvalk-ov previously approved these changes Oct 15, 2021
View reviewed changes
@dtvalk-ov dtvalk-ov requested a review from a team October 15, 2021 13:07
@martin-stanchev martin-stanchev force-pushed the feature/UPPSF-2731-lists-push-notifications branch 3 times, most recently from be54097 to b4c6e22 Compare October 19, 2021 09:47
atanasdinov
atanasdinov previously approved these changes Oct 19, 2021
View reviewed changes
@martin-stanchev martin-stanchev force-pushed the feature/UPPSF-2731-lists-push-notifications branch from b4c6e22 to 21ecde3 Compare October 20, 2021 08:09
@dtvalk-ov dtvalk-ov requested review from atanasdinov and a team October 20, 2021 08:10
@martin-stanchev martin-stanchev merged commit 9baccf2 into master Oct 20, 2021
@martin-stanchev martin-stanchev deleted the feature/UPPSF-2731-lists-push-notifications branch October 20, 2021 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atanasdinov atanasdinov atanasdinov approved these changes

@dtvalk-ov dtvalk-ov dtvalk-ov approved these changes

@ivanruski ivanruski ivanruski left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@martin-stanchev @coveralls @ivanruski @atanasdinov @dtvalk-ov