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

Enh: First version of the notifications service #3217

Merged
merged 4 commits into from
Feb 24, 2022
Merged

Conversation

C0rby
Copy link
Contributor

@C0rby C0rby commented Feb 22, 2022

Description

Implemented the minimal version of the notifications service to be able to notify a user when they received a share.
Currently the message is hard coded and there is only one communication channel for email. In the future we could have templated messages which could also be formatted by the the communication channel. Also a HTTP/REST service could be added to allow other services to send messages to users.

I implemented the functionality to also send messages to groups but this doesn't work currently since we don't have an authenticated context which we need to communicate with the gateway. On solution for this would be to have a service user but maybe we could have other solutions like mTLS for service to service communication?

@micbar should we tackle this in this PR still?

Motivation and Context

oCIS needs to be able to notify users in certain situations. The notifications service provides methods to communicate to the users via various channels.

How Has This Been Tested?

  • test environment:
  • Locally using MailHog as the mail server.

Screenshots (if appropriate):

Screenshot of received mail

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)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

@C0rby C0rby added the Status:Needs-Review Needs review from a maintainer label Feb 22, 2022
@C0rby C0rby self-assigned this Feb 22, 2022
@ownclouders
Copy link
Contributor

ownclouders commented Feb 22, 2022

💥 Acceptance test Web-Tests-ocis-smoke-ocis-storage-2 failed. Further test are cancelled...

notifications/pkg/service/service.go Outdated Show resolved Hide resolved
@sonarcloud
Copy link

sonarcloud bot commented Feb 23, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

1.2% 1.2% Coverage
22.3% 22.3% Duplication

Copy link
Collaborator

@kobergj kobergj left a comment

Choose a reason for hiding this comment

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

Looks good. I would approve if it wasn't a draft...

@C0rby
Copy link
Contributor Author

C0rby commented Feb 23, 2022

Yeah, the question is what should we do about the groups topic?
@micbar, should we just merge it as is, i.e. group notifications don't work at the moment or introduce a new service user which will be used by the notifications service?

@micbar
Copy link
Contributor

micbar commented Feb 23, 2022

I am for merging now. Group functionality should go into refinement.

@C0rby C0rby marked this pull request as ready for review February 24, 2022 08:22
@micbar
Copy link
Contributor

micbar commented Feb 24, 2022

@kobergj now?

@C0rby C0rby merged commit 8b2ee7a into master Feb 24, 2022
@delete-merged-branch delete-merged-branch bot deleted the notifications-service branch February 24, 2022 08:53
ownclouders pushed a commit that referenced this pull request Feb 24, 2022
Merge: 4bc85c9 e160615
Author: David Christofas <dchristofas@owncloud.com>
Date:   Thu Feb 24 09:53:34 2022 +0100

    Merge pull request #3217 from owncloud/notifications-service

    Enh: First version of the notifications service
@micbar micbar mentioned this pull request Mar 3, 2022
26 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status:Needs-Review Needs review from a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants