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

#161967021 Implement the notification system #34

Merged
merged 1 commit into from
Dec 21, 2018

Conversation

babbageLabs
Copy link
Contributor

@babbageLabs babbageLabs commented Dec 20, 2018

What does this PR do?

It implements the notifications endpoint and email notifications
The notifications generated are for:

  • New articles published by authors one follows
  • New comments on articles one has favourited
  • New Followers
  • New favourites on articles one has published

Description of Task to be completed?

USers are able to ::

  1. check or update their mailing status
    GET /api/mailing_status/
    PUT /api/mailing_status/

  2. View the Mailing List
    GET /api/mailing_List/

  3. check read and unread notifications
    GET /api/notifications/unread
    GET /api/notifications/read

  4. read and unmark a specific notification as read
    GET /api/notifications/<int:id>
    GET /api/notifications/unmark/<int:id>

  5. Mark all notifications as read
    GET /api/notifications/mark_read

How should this be manually tested?

 git clone https://github.com/andela/ah-jumanji.git
 cd ah-jumanji
 git checkout ft-notifications-system-161967021
 virtualenv venv
 source ./bin/activate
 pip install -r requirements.txt
 touch .env
 echo "DATABASE_URL=postgres://username:password@host/db_name" >> .env
 echo "PROTOCOL=https" >> .env
 echo "EMAIL_HOST_USER=email@email.com" >> .env
echo " EMAIL_HOST_PASSWORD=password" >> .env
 echo "DEBUG=on" >> .env
 python manage.py migrate
 python manage.py createcachetable 
 python manage.py qcluster #This is the cluser service and must be run in the background
 python manage.py qmonitor #This monitors the background service and running it is optional
 python manage.py runserver

Any background context you want to provide?

  • The project employs django-q as an asyscronous task backend and DjangoORM/Postgres as the broker. This reliance on third party packages is to make sure emails can be sent asyncronously without halting server requests execution.

  • Notification responses contain ;

    • Actor. The object that performed the activity.
    • Verb. The verb phrase that identifies the action of the activity.
    • Action Object. (Optional) The object linked to the action itself.
    • Target. (Optional) The object to which the activity was performed.
    • timestamp
    • timesince -time since the notification was posted to now
    • emailed -boolean of whether the notification has been emailed or not
    • unread - boolean of the read status of the notification

They can be decoded as shown in the following example:
justquick (actor) closed (verb) issue 2 (action_object) on activity-stream (target) 12 hours ago

What are the relevant pivotal tracker stories?

#161967021

Screenshots (if appropriate)

Django Cluster service
image
Django cluster monitoring service
image

Mailing List
image

Mailing List status
image

notifications
image

image

Checklist:

  • My code follows the style guidelines of this project
  • At least 2 people have reviewed my PR
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • My PR has one commit.

@babbageLabs babbageLabs requested review from GransonO, mashafrancis and gwako94 and removed request for GransonO and mashafrancis December 20, 2018 11:58

message = """
You have a new follower @%s .

Choose a reason for hiding this comment

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

blank line contains whitespace

# Create your views here.
import logging

from django.contrib.auth import get_user_model

Choose a reason for hiding this comment

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

'django.contrib.auth.get_user_model' imported but unused

# create an article for user2
ArticlesFactory(author=user2)

notification = Notification.objects.all()[0]

Choose a reason for hiding this comment

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

local variable 'notification' is assigned to but never used

@@ -0,0 +1,20 @@
from django.urls import path

from authors.apps.notifier.views import GetMailingList, ViewUpdateMailingList, \

Choose a reason for hiding this comment

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

line too long (80 > 79 characters)

@@ -0,0 +1,3 @@
from django.contrib import admin

Choose a reason for hiding this comment

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

'django.contrib.admin' imported but unused

@@ -0,0 +1,3 @@
from django.test import TestCase

Choose a reason for hiding this comment

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

'django.test.TestCase' imported but unused

subject = "New Article favourite Notification"
Message = """
Your article %s has been favourited by %s.

Choose a reason for hiding this comment

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

blank line contains whitespace

@@ -0,0 +1,3 @@
from django.contrib import admin

Choose a reason for hiding this comment

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

'django.contrib.admin' imported but unused

You are seeing this email because you are subscribed to /n
receive email notifications.To unsubscribe from this emails
login and unsubscribe by following %s

Choose a reason for hiding this comment

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

blank line contains whitespace

subject = "New Publication Notification"
message = """
%s published a new article.Check it out at %s .

Choose a reason for hiding this comment

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

blank line contains whitespace

@babbageLabs babbageLabs force-pushed the ft-notifications-system-161967021 branch 4 times, most recently from f7b88dc to a3b8e3e Compare December 20, 2018 17:13
@babbageLabs babbageLabs temporarily deployed to ah-jumanji-staging December 20, 2018 18:16 Inactive
@babbageLabs babbageLabs force-pushed the ft-notifications-system-161967021 branch from a3b8e3e to 7eabb8e Compare December 20, 2018 19:04
@babbageLabs babbageLabs temporarily deployed to ah-jumanji-staging December 20, 2018 19:09 Inactive
Copy link
Contributor

@mashafrancis mashafrancis left a comment

Choose a reason for hiding this comment

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

The functionality works in an epic way. Good work!
I have tested the functionality and is good with me.

@babbageLabs babbageLabs temporarily deployed to ah-jumanji-staging December 21, 2018 06:41 Inactive
@fabzer0
Copy link
Contributor

fabzer0 commented Dec 21, 2018

Nice work!

@babbageLabs babbageLabs force-pushed the ft-notifications-system-161967021 branch from 7eabb8e to 5d402c8 Compare December 21, 2018 08:09
Copy link
Contributor

@gwako94 gwako94 left a comment

Choose a reason for hiding this comment

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

The pr works fine. This can be merged.

@babbageLabs babbageLabs force-pushed the ft-notifications-system-161967021 branch 3 times, most recently from b5c8ef7 to dbfc617 Compare December 21, 2018 10:22
 - add unit tests
 - implement notifications system
 - set up an asyncronuos task queing system
 - update the Read.me
 - Update requirements.txt
 - Update the .procfile for monitoring of the asyncronous tasks
@babbageLabs babbageLabs force-pushed the ft-notifications-system-161967021 branch from dbfc617 to 4fea9ce Compare December 21, 2018 10:30
@kipropbrian kipropbrian merged commit 350ddf7 into develop Dec 21, 2018
@kipropbrian kipropbrian deleted the ft-notifications-system-161967021 branch December 21, 2018 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants