-
Notifications
You must be signed in to change notification settings - Fork 19
Notifications - issue #165 #240
Changes from all commits
9ac8daf
c71b0c5
93755bf
c240e5c
48303ef
63e9e79
feb6dbc
04ecad0
b63ad70
dd130f9
dc52c17
70cbcd2
3713e99
dcba65b
f6bb9c8
41c32ca
0f66119
e5909e1
fecf0b6
2268613
98ca54a
8c34061
b1059a0
8d9b496
750cde1
c59df26
db6555a
d799a04
a7f203d
8b134bd
2bc2865
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,4 +27,4 @@ celerybeat-schedule | |
*.celerybeat-schedule | ||
|
||
# Celerybeat | ||
*.pid | ||
*.pid | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,8 @@ | |
import mongoengine | ||
from mongoengine.django import auth | ||
|
||
import uuid | ||
|
||
from . import fields, utils | ||
from .. import panels | ||
|
||
|
@@ -27,6 +29,9 @@ def upper_birthdate_limit(): | |
def lower_birthdate_limit(): | ||
return timezone.now().date() - datetime.timedelta(LOWER_DATE_LIMIT) | ||
|
||
def generate_channel_id(): | ||
return uuid.uuid4() | ||
|
||
class Connection(mongoengine.EmbeddedDocument): | ||
http_if_none_match = mongoengine.StringField() | ||
http_if_modified_since = mongoengine.StringField() | ||
|
@@ -62,6 +67,7 @@ class User(auth.User): | |
birthdate = fields.LimitedDateTimeField(upper_limit=upper_birthdate_limit, lower_limit=lower_birthdate_limit) | ||
gender = fields.GenderField() | ||
language = fields.LanguageField() | ||
channel_id = mongoengine.UUIDField(binary=False, default=generate_channel_id) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, you are using UUIDField. Then you should be using uuid value as a default. You can use random uuid for this. (See uuid Python package.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. |
||
|
||
facebook_access_token = mongoengine.StringField(max_length=150) | ||
facebook_profile_data = mongoengine.DictField() | ||
|
@@ -155,6 +161,9 @@ def get_image_url(self): | |
else: | ||
return staticfiles_storage.url(settings.DEFAULT_USER_IMAGE) | ||
|
||
def get_user_channel(self): | ||
return "user/%s" % self.channel_id | ||
|
||
@classmethod | ||
def create_user(cls, username, email=None, password=None): | ||
now = timezone.now() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,4 +8,13 @@ def apply_limits(self, request, object_list): | |
else: | ||
object_list = object_list.filter(is_published=True) | ||
|
||
return object_list | ||
|
||
class NotificationAuthorization(tastypie_authorization.Authorization): | ||
def apply_limits(self, request, object_list): | ||
if request and hasattr(request, 'user'): | ||
object_list = object_list.filter(recipient=request.user) | ||
else: | ||
object_list = [] | ||
|
||
return object_list | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not good. Otherwise it should return an empty list. So you return filtered query set if user information is available, otherwise you return an empty list. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,8 @@ | |
|
||
import mongoengine | ||
|
||
from pushserver import utils | ||
from piplmesh.account import models as account_models | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Import with relative import as you can see bellow. |
||
from . import base | ||
|
||
POST_MESSAGE_MAX_LENGTH = 500 | ||
|
@@ -33,6 +35,8 @@ class Post(base.AuthoredDocument): | |
comments = mongoengine.ListField(mongoengine.EmbeddedDocumentField(Comment), default=lambda: [], required=False) | ||
attachments = mongoengine.ListField(mongoengine.EmbeddedDocumentField(Attachment), default=lambda: [], required=False) | ||
|
||
subscribers = mongoengine.ListField(mongoengine.ReferenceField(account_models.User), default=lambda: [], required=False) | ||
|
||
# TODO: Prevent posting comments if post is not published | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Empty line between subscribers and TODOs. |
||
# TODO: Prevent adding attachments if post is published | ||
# TODO: Prevent marking post as unpublished once it was published | ||
|
@@ -42,6 +46,19 @@ def save(self, *args, **kwargs): | |
self.updated_time = timezone.now() | ||
return super(Post, self).save(*args, **kwargs) | ||
|
||
class Notification(mongoengine.Document): | ||
""" | ||
This class defines document type for notifications. | ||
""" | ||
|
||
recipient = mongoengine.ReferenceField(account_models.User, required=True) | ||
created_time = mongoengine.DateTimeField(default=timezone.now, required=True) | ||
read = mongoengine.BooleanField(default=False) | ||
post = mongoengine.ReferenceField(Post) | ||
|
||
# TODO: This is probably not the best approach. | ||
comment = mongoengine.IntField() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, int is not good ID. But OK, use this for now, but add TODO for later, maybe somebody else can improve this later. |
||
|
||
class UploadedFile(base.AuthoredDocument): | ||
""" | ||
This class document type for uploaded files. | ||
|
@@ -65,4 +82,4 @@ class LinkAttachment(Attachment): | |
|
||
link_url = mongoengine.URLField(required=True) | ||
link_caption = mongoengine.StringField(default='', required=True) | ||
link_description = mongoengine.StringField(default='', required=True) | ||
link_description = mongoengine.StringField(default='', required=True) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,11 @@ | ||
from django.conf import settings | ||
|
||
from tastypie import authorization as tastypie_authorization, fields as tastypie_fields | ||
|
||
from tastypie_mongoengine import fields, paginator, resources | ||
|
||
from pushserver.utils import updates | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need, because they are all part of the same package. |
||
from piplmesh.account import models as account_models | ||
from piplmesh.api import authorization, models as api_models, signals | ||
|
||
|
@@ -26,12 +30,41 @@ def hydrate(self, bundle): | |
return bundle | ||
|
||
class CommentResource(AuthoredResource): | ||
def obj_create(self, bundle, request=None, **kwargs): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please read my comments here: https://github.com/wlanslovenija/PiplMesh/pull/199/files#r1366928 So the main point is that you should decouple API from frontend. So in the API you just deal with storing data (so storing to list of subscribers is OK). But to send something for frontend ( So please define and use Django signals. And move the code for frontend to frontend. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And same as for discussion above, be careful where you put those signals. So adding subscribers is a part of creating an object. But sending notifications should come later, when we are really sure everything was done properly and we are just about to return the response. |
||
bundle = super(CommentResource, self).obj_create(bundle, request=request, **kwargs) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, still, you should define a new signal here and then hook onto it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Forget it. It is OK like this. We can define notifications as part of the API. Also expose notifications through RESTful API. |
||
for subscriber in self.instance.subscribers: | ||
if subscriber != bundle.obj.author: | ||
notification = api_models.Notification.objects.create(recipient=subscriber, post=self.instance, comment=bundle.obj.pk) | ||
signals.notification_created.send(sender=self, notification=notification, request=request or bundle.request) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. MongoEngine already defines signals for documents. Maybe this is what you are searching for here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried to use mongoengine signals and Post_save method in api.models.py, but there is a problem because i can't import NotificationResources in models.py because of circular imports. Because of that i cannot use serialize from NotificationResources and have to manually create json object using python json package. This new code is working but is not so clean as current one. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But why you want to implement code in models.py? Why not views.py of frontend? The same place where the previous code was? That's the ideas of signals, you can implement them anywhere. But the question is if that defined signals are good enough. I am not so sure because they are missing resource and bundle. So probably we have to implement our own signals for this case. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (As you did.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (But you have to understand why is necessary to have your own signal. Not because of the location. But because of missing other objects.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes i understand. I noticed that when using mongoengine signals we are missing objects i just didn't mention it in the comment. That was main reason why i asked if we really wanted to use mongoengine signal. So should i use django signals instead as i did before because there were located in frontend/views.py and for most important part we had all the object needed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
And I wanted that you argument why they might not be good. :-)
Both MongoEngine and ours are Django signals. :-)
This is completely your decision where you decide to connect onto them. There is no reason why you did it in models.py! See, MongoEngine signals are nothing different to signals we do, they are just defined in MongoEngine and called there. Where you connect to them, this is your decision.
This is the most important part. It allows us to same serialization as we are using for REST protocol. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I did :).
I also pointed this out in my first comment when i changed to signals defined in mongoengine. Signals are now fixed, serialized as we are using for REST protocol and have all the objects that might need in the future. |
||
|
||
if bundle.obj.author not in self.instance.subscribers: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This part can stay here. So when comment is created, author is automatically added to the list of subscribers. This is something which is part of the resource-handling, yes. |
||
self.instance.subscribers.append(bundle.obj.author) | ||
self.instance.save() | ||
|
||
return bundle | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One empty line before this. |
||
|
||
class Meta: | ||
object_class = api_models.Comment | ||
allowed_methods = ('get', 'post', 'put', 'patch', 'delete') | ||
# TODO: Make proper authorization, current implementation is for development use only | ||
authorization = tastypie_authorization.Authorization() | ||
|
||
class NotificationResource(resources.MongoEngineResource): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Configure other fields read-only. |
||
comment_message = tastypie_fields.CharField(default='', null=False, blank=True) | ||
comment_author = tastypie_fields.CharField(default='', null=False, blank=True) | ||
|
||
def dehydrate_comment_author(self, bundle): | ||
return bundle.obj.post.comments[bundle.obj.comment].author | ||
|
||
def dehydrate_comment_message(self, bundle): | ||
return bundle.obj.post.comments[bundle.obj.comment].message | ||
|
||
class Meta: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are doing only There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will also add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, if you will be also setting read field. But of course, API should allow only setting read field. You might to configure other fields read-only. |
||
queryset = api_models.Notification.objects.all() | ||
allowed_methods = ('get') | ||
authorization = authorization.NotificationAuthorization() | ||
|
||
class ImageAttachmentResource(AuthoredResource): | ||
image_file = fields.ReferenceField(to='piplmesh.api.resources.UploadedFileResource', attribute='image_file', null=False, full=True) | ||
image_description = tastypie_fields.CharField(attribute='image_description', default='', null=False, blank=True) | ||
|
@@ -74,11 +107,13 @@ class PostResource(AuthoredResource): | |
|
||
def obj_create(self, bundle, request=None, **kwargs): | ||
bundle = super(PostResource, self).obj_create(bundle, request=request, **kwargs) | ||
bundle.obj.subscribers.append(bundle.request.user) | ||
bundle.obj.save() | ||
signals.post_created.send(sender=self, post=bundle.obj, request=request or bundle.request, bundle=bundle) | ||
return bundle | ||
|
||
class Meta: | ||
queryset = api_models.Post.objects.all().order_by('-updated_time') | ||
allowed_methods = ('get', 'post', 'put', 'patch', 'delete') | ||
authorization = authorization.PostAuthorization() | ||
paginator_class = paginator.Paginator | ||
paginator_class = paginator.Paginator |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
from django import dispatch | ||
|
||
post_created = dispatch.Signal(providing_args=('post', 'request', 'bundle')) | ||
notification_created = dispatch.Signal(providing_args=('notification', 'request')) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add also bundle. You never know what is useful to others. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -197,6 +197,67 @@ function showLastPosts(offset) { | |
}); | ||
} | ||
|
||
function addNewNotification(newNotification) { | ||
var notification_counter = parseInt($('#notifications_count').text()) + 1; | ||
$('#notifications_count').html(notification_counter); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed all that was mentioned above. I thing everything is set for pull. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You have not changed to |
||
$('.notification_list').prepend(buildNotification(newNotification.notification)); | ||
} | ||
|
||
function buildNotification(notification) { | ||
var format = gettext("%(author)s commented on post."); | ||
var author = interpolate(format, {'author': notification.comment_author}, true); | ||
|
||
var new_notification = $('<li/>').addClass('notification').append( | ||
$('<span/>').addClass('notification_element').text(author) | ||
).append( | ||
$('<span/>').addClass('notification_message').addClass('notification_element').text(notification.comment_message) | ||
).append( | ||
$('<span/>').addClass('notification_element').addClass('notification_created_time').text(formatDiffTime(notification.created_time)) | ||
); | ||
new_notification.data('notification', notification); | ||
|
||
return new_notification; | ||
} | ||
|
||
function updateNotificationDate(element) { | ||
$(element).find('.notification_created_time').text(formatDiffTime(element.data('notification').created_time)); | ||
} | ||
|
||
function loadNotifications() { | ||
$.getJSON(URLS.notifications, function (notifications, textStatus, jqXHR) { | ||
var unread_counter = 0; | ||
|
||
var content = $('<ul/>').addClass('notification_list'); | ||
|
||
$.each(notifications.objects, function (i, notification) { | ||
if (!notification.read) { | ||
unread_counter++; | ||
} | ||
content.prepend(buildNotification(notification)); | ||
}) | ||
|
||
$('#notifications_content').html(content); | ||
$('#notifications_count').text(unread_counter); | ||
}); | ||
} | ||
|
||
// This is just for testing purposes. It can be base for future development. | ||
function addComment(comment) { | ||
// TODO: Change this for any post | ||
var post_url = $('.post').first().data('post').resource_uri; | ||
|
||
$.ajax({ | ||
type: 'POST', | ||
url: post_url + 'comments/', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mnja. Let it be. :-) But you should add TODO, that it should not be hardcoded like this. :-) |
||
data: JSON.stringify({'message': comment}), | ||
contentType: 'application/json', | ||
dataType: 'json', | ||
success: function (data, textStatus, jqXHR) { | ||
alert("Comment posted."); | ||
}, | ||
}); | ||
} | ||
|
||
$(document).ready(function () { | ||
initializePanels(); | ||
|
||
|
@@ -216,6 +277,21 @@ $(document).ready(function () { | |
}); | ||
}); | ||
|
||
// Notifications | ||
$('#notifications_count').click(function () { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TODO. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You haven't added |
||
$('#notifications_box').slideToggle('fast'); | ||
}); | ||
$('.close_notifications_box').click(function (event) { | ||
$('#notifications_box').slideToggle('fast'); | ||
}); | ||
$('#add_comment').click(function (event) { | ||
addComment("Test comment"); | ||
}); | ||
|
||
$.updates.registerProcessor('user_channel', 'notification', addNewNotification); | ||
|
||
loadNotifications(); | ||
|
||
// TODO: Ajax request to store panels state is currently send many times while resizing, it should be send only at the end | ||
$(window).resize(function (event) { | ||
initializePanels(); | ||
|
@@ -283,5 +359,8 @@ $(document).ready(function () { | |
$('.post').each(function (i, post) { | ||
$(post).data('post').updateDate(this); | ||
}); | ||
$('.notification').each(function (i, notification) { | ||
updateNotificationDate($(notification)); | ||
}); | ||
}, POSTS_DATE_UPDATE_INTERVAL); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,6 +46,10 @@ def check_online_users(): | |
connections__in=([], None), # None if field is missing altogether | ||
connection_last_unsubscribe__lt=timezone.now() - datetime.timedelta(seconds=CHECK_ONLINE_USERS_RECONNECT_TIMEOUT), | ||
).update(set__is_online=False): | ||
user.reload() | ||
# On user disconnect new channel_id is generated for safety reasons | ||
user.channel_id = models.generate_channel_id() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Some comment would be really good here, why and what exactly we are doing here. So why exactly here, what is this. |
||
user.save() | ||
updates.send_update( | ||
views.HOME_CHANNEL_ID, | ||
{ | ||
|
@@ -56,4 +60,4 @@ def check_online_users(): | |
'image_url': user.get_image_url(), | ||
}, | ||
} | ||
) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any particular reason that you are removing those lines? For different platforms we have observed different files so this is why all this is here. And I don't think there are minuses to having this here. Why then removing?