Notification framework by MridulS · Pull Request #191 · Murali-group/GraphSpace

MridulS · 2016-07-22T17:59:27Z

Screen after logging in

Click on notifications in the nav bar.

tmmurali · 2016-07-24T01:01:37Z

graphs/models.py

    #no relationship specified

+class share_graph_event(Base):
+    __tablename__ = 'share_graph_event'


Need some docs on the schema. What does the table store? Why do you need each attribute?

Class names should have camel case.

tmmurali · 2016-07-24T01:11:21Z

Looks very promising. I added some comments.

On Fri, Jul 22, 2016 at 1:59 PM, Mridul Seth notifications@github.com
wrote:

Screen after logging in
[image: screen shot 2016-07-22 at 7 04 45 pm]
https://cloud.githubusercontent.com/assets/5363860/17066269/9244a5aa-5046-11e6-96ba-b0f5506cf133.png
Click on notifications in the nav bar.
[image: screen shot 2016-07-22 at 7 04 53 pm]
https://cloud.githubusercontent.com/assets/5363860/17066271/958f8018-5046-11e6-81f6-2dcefcc85596.png

@adbharadwaj https://github.com/adbharadwaj @tmmurali

https://github.com/tmmurali

You can view, comment on, or merge this pull request online at:

#191
Commit Summary

Add initial layout for notifications framework

Add spec for share graph event db functions

Add boilerplate test

File Changes

M graphs/models.py
https://github.com/Murali-group/GraphSpace/pull/191/files#diff-0
(12)

M graphs/templates/base.html
https://github.com/Murali-group/GraphSpace/pull/191/files#diff-1 (5)

A graphs/templates/graphs/notifications.html
https://github.com/Murali-group/GraphSpace/pull/191/files#diff-2
(39)

M graphs/urls.py
https://github.com/Murali-group/GraphSpace/pull/191/files#diff-3 (3)

M graphs/util/db.py
https://github.com/Murali-group/GraphSpace/pull/191/files#diff-4
(59)

A graphs/util/db_test.py
https://github.com/Murali-group/GraphSpace/pull/191/files#diff-5
(12)

M graphs/views.py
https://github.com/Murali-group/GraphSpace/pull/191/files#diff-6 (9)

Patch Links:

https://github.com/Murali-group/GraphSpace/pull/191.patch

https://github.com/Murali-group/GraphSpace/pull/191.diff

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#191, or mute the thread
https://github.com/notifications/unsubscribe-auth/AGkWUBOC8eQHgG475b7Pmpk98kkxm5CJks5qYQT_gaJpZM4JS_ZW
.

adbharadwaj · 2016-07-25T15:41:21Z

graphs/views.py

@@ -297,6 +297,18 @@ def _graphs_page(request, view_type):

    return render(request, 'graphs/graphs.html', context)



Add python documentation for this method. https://www.python.org/dev/peps/pep-0257/

adbharadwaj · 2016-07-25T17:14:46Z

@MridulS, I have updated the master branch with new installation process.

I would like you to fetch your master branch from main repository and update your master branch.
Then merge your master into your branch.
Follow the new installation procedure and let me know if you are facing any problems.

adbharadwaj · 2016-07-29T23:35:08Z

graphs/util/db.py

+	db_session = data_connection.new_session()
+
+	# Get the current time
+	cur_time = datetime.now()


we dont need this variable. you directly assign the value while creating the share event object.

adbharadwaj · 2016-07-30T00:25:34Z

I added some comments. The priority is to get a working demo by Monday meeting though.

adbharadwaj · 2016-08-05T20:07:04Z

graphs/exception.py

@@ -0,0 +1,4 @@
+from django.core.exceptions import ObjectDoesNotExist
+


Not to be used anymore.

MridulS · 2016-08-05T22:42:55Z

@tmmurali @adbharadwaj Updates to the front end.

acc@1.com shares 2 graphs in group groupfromuser1 and 3 graphs in group secondgroupofuser1 and acc@2.com is part of both the groups.
After logging in with acc@2.com and navigating to notifications.

Click on Mark as read for one notification in the first group

Click on Mark all as read for the second group

Click on Mark all notifications as read

Click on groupfromuser1 bar in the left column. This will give all the notifications generated in that group.

Similarly for the second group.

adbharadwaj · 2016-08-08T16:18:14Z

graphs/util/db.py

+	db_session = data_connection.new_session()
+	try:
+
+		events = db_session.query(models.ShareGraphEvent).filter(models.ShareGraphEvent.member_id == member_id).filter(models.ShareGraphEvent.group_id == group_id).all()


Can you use the update api here ?

adbharadwaj · 2016-08-08T17:54:57Z

4.1 Notification system

4.1.1 View all notifications flow.

User Request: <host>/<uid>/notifications
Views: notifications method
1. Controller: get_notifications_for_user(user_id, group_id=None)
  1. DAL: get_share_graph_event_by_member_id()
2. Controller: Handle exceptions like user doesnt exist or other kinds of database errors. Communicate these errors to view layer so that appropriate messages can be shown to the users.
Views: Construct context for templates.
Views: Render template (notifications.html) and send to user.
User Response: HTML page.

4.1.2 View all notifications for a group.

User Request: <host>/<uid>/notifications?group_id=<group_id>
Views: notifications_method
1. Controller: get_notifications_for_user(user_id, group_id)
  1. DAL: get_share_graph_event_by_member_id_and_group_id(member_id, group_id)
2. Controller: Handle exceptions like user/group doesnt exist or user is not a member of the group or other kinds of database errors. Communicate these errors to view layer so that appropriate messages can be shown to the users.
Views: Construct context for templates.
Views: Render template (notifications.html) and send to user.
User Response: HTML page.

4.1.3 Mark one or more notifications as read.

User Action: User clicks on one of the ticks or "mark all as read" button.
Javascript: Catches users action. Fetches the corresponding notification_id(s). Creates the POST request and sends a AJAX request.
AJAX Request: <host>/javascript/<uid>/mark_notifications_as_read/ It will be a POST API request with list of notification ids in the request body.
Views: mark_notifications_as_read_api(user_id, notification_ids).
1. Controller: mark_notifications_as_read()
  1. DAL: set_share_graph_events_inactive()
2. Controller: Handle exceptions like user/share_graph_event doesnt exist or other kinds of database errors. Communicate these errors to view layer so that appropriate messages can be shown to the users.
Views: Construct context for JSON response.
Views: Send JSON response to client side javascript.
AJAX Response: Check if the request was successfull. If successfull, then gray up the notification row and remove the tick mark by manipulating the DOM.

MridulS · 2016-08-14T19:39:47Z

I have made two different User Interfaces right now, one which greys out the notifications as the tick mark button is clicked (you need to refresh to remove those notifications from the screen) and the other one removes the notification from the list of notification on click.

The greying out one:

The other one:

The grey one is closer to github but I personally like the second one.

@tmmurali @adbharadwaj Thoughts?

adbharadwaj · 2016-08-15T19:45:45Z

graphs/util/db.py

+		return {'Error': 'No share graph event found.'}
+
+
+def get_share_graph_event_by_member_id_and_group_id(member_id, group_id):


Docstring required

…ifications.html

MridulS · 2016-08-17T13:27:19Z

@adbharadwaj The left group column uses groups from grouped_notifications.keys() to fetch the groups, if all the notifications are read then it will not be possible to fetch groups for the user so I have added a new controller method get_notifications_group_stats_for_user which fetches all the stats from the db to create the left column in notifications.html

adbharadwaj · 2016-08-17T15:08:03Z

graphs/static/graphs/js/notifications.js


-    $('[data-toggle="tooltip"]').tooltip();
+    // grey out notification row and remove the tick mark for notification row
+    var manipulate_dom = function(notification_ids){


rename method name to "grey_out_notifications"

MridulS added 3 commits July 22, 2016 19:02

Add initial layout for notifications framework

dc5e6f8

Add spec for share graph event db functions

3d9a85d

Add boilerplate test

c79c1bd

tmmurali reviewed Jul 24, 2016
View reviewed changes

Add error page if user is not logged in for notifications

5cd7500

adbharadwaj reviewed Jul 25, 2016
View reviewed changes

MridulS added 3 commits July 28, 2016 18:08

Update ShareGraphEvent model and implement functions in db.py

d507c44

update login error text

1dd3d6b

create new event after sharing graph with group

47feb76

adbharadwaj reviewed Jul 29, 2016
View reviewed changes

MridulS added 4 commits August 1, 2016 01:04

add support for reading notifications

0e9465f

add boilerplate exceptions file

6982837

Add functionality to read all events of a group

ec85ee2

update notifications template

f630f8a

adbharadwaj reviewed Aug 5, 2016
View reviewed changes

graphs/exception.py Outdated

@@ -0,0 +1,4 @@

from django.core.exceptions import ObjectDoesNotExist

Copy link

Collaborator

adbharadwaj Aug 5, 2016

Choose a reason for hiding this comment

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

Not to be used anymore.

add functionality to view all notifications of a group

4c24e6c

adbharadwaj reviewed Aug 8, 2016
View reviewed changes

MridulS added 4 commits August 11, 2016 19:13

changes

db07aa5

Merge branch 'master' into notification

e98ff6f

Update notifications code according to the spec

99c3aff

update UI for notifications

80124a4

adbharadwaj reviewed Aug 15, 2016
View reviewed changes

adbharadwaj and others added 7 commits August 16, 2016 11:59

removing unnecessary packages

7d5a2c9

refactoring get all notifications flow-still incomplete

24ea0e2

refactored notifications flow

3706261

Update notifications.js according to notifications spec

9c672dd

Add group information and the number badges to the left column in not…

3542ff8

…ifications.html

update urls.py to point to correct view

8cd3d09

Update views and db to match the notifications spec

d9f31bb

Remove unnecessary file get-pip

d4f786a

adbharadwaj reviewed Aug 17, 2016
View reviewed changes

		@@ -297,6 +297,18 @@ def _graphs_page(request, view_type):

		return render(request, 'graphs/graphs.html', context)

		@@ -0,0 +1,4 @@
		from django.core.exceptions import ObjectDoesNotExist

		return {'Error': 'No share graph event found.'}


		def get_share_graph_event_by_member_id_and_group_id(member_id, group_id):

Conversation

MridulS commented Jul 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmmurali commented Jul 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adbharadwaj commented Jul 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adbharadwaj commented Jul 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MridulS commented Aug 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adbharadwaj commented Aug 8, 2016

4.1 Notification system

4.1.1 View all notifications flow.

4.1.2 View all notifications for a group.

4.1.3 Mark one or more notifications as read.

Uh oh!

MridulS commented Aug 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MridulS commented Aug 17, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants