Skip to content
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
dc5e6f8
Add initial layout for notifications framework
MridulS Jul 22, 2016
3d9a85d
Add spec for share graph event db functions
MridulS Jul 22, 2016
c79c1bd
Add boilerplate test
MridulS Jul 22, 2016
5cd7500
Add error page if user is not logged in for notifications
MridulS Jul 24, 2016
d507c44
Update ShareGraphEvent model and implement functions in db.py
MridulS Jul 28, 2016
1dd3d6b
update login error text
MridulS Jul 28, 2016
47feb76
create new event after sharing graph with group
MridulS Jul 28, 2016
0e9465f
add support for reading notifications
MridulS Jul 31, 2016
6982837
add boilerplate exceptions file
MridulS Aug 1, 2016
ec85ee2
Add functionality to read all events of a group
MridulS Aug 1, 2016
f630f8a
update notifications template
MridulS Aug 5, 2016
4c24e6c
add functionality to view all notifications of a group
MridulS Aug 5, 2016
db07aa5
changes
MridulS Aug 11, 2016
e98ff6f
Merge branch 'master' into notification
MridulS Aug 11, 2016
99c3aff
Update notifications code according to the spec
MridulS Aug 14, 2016
80124a4
update UI for notifications
MridulS Aug 14, 2016
7d5a2c9
removing unnecessary packages
adbharadwaj Aug 16, 2016
24ea0e2
refactoring get all notifications flow-still incomplete
adbharadwaj Aug 16, 2016
3706261
refactored notifications flow
adbharadwaj Aug 16, 2016
9c672dd
Update notifications.js according to notifications spec
MridulS Aug 17, 2016
3542ff8
Add group information and the number badges to the left column in not…
MridulS Aug 17, 2016
8cd3d09
update urls.py to point to correct view
MridulS Aug 17, 2016
d9f31bb
Update views and db to match the notifications spec
MridulS Aug 17, 2016
d4f786a
Remove unnecessary file get-pip
MridulS Aug 17, 2016
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 29 additions & 2 deletions graphs/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@
ex. 'id' for user table would be 'user_id'
'''

from sqlalchemy import Column, Integer, String, ForeignKey, Table, Index, ForeignKeyConstraint
from sqlalchemy import Column, Integer, String, ForeignKey, Table, Index, ForeignKeyConstraint, Boolean, UniqueConstraint
from sqlalchemy.ext.declarative import declarative_base
from sqlalchemy.orm import relationship, backref
from sqlalchemy.types import TIMESTAMP
from sqlalchemy.types import TIMESTAMP, Boolean
from django.db import models
from django.conf import settings
from sqlalchemy import create_engine
Expand Down Expand Up @@ -310,6 +310,33 @@ class Edge(Base):
ForeignKeyConstraint([user_id, graph_id, tail_node_id], [Node.user_id, Node.graph_id, Node.node_id], ondelete="CASCADE", onupdate="CASCADE"), {})
#no relationship specified

class ShareGraphEvent(Base):
__tablename__ = 'share_graph_event'
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Class names should have camel case.

# unique id for each share graph event
id = Column(Integer, autoincrement=True, primary_key=True)
# id of the graph shared
graph_id = Column(String, nullable=False)
# id of the owner of the graph which is shared
owner_id = Column(String, nullable=False)
# id of the group the graph is shared in
group_id = Column(String, ForeignKey('group.group_id', ondelete="CASCADE", onupdate="CASCADE"), nullable=False)
# id of the member of the group.
# Hence there can be multiple share graph events if a owner shares a grap
# with a group. A share graph event will be created for all the memebers
# of the group except the owner of the graph (the one who shared it).
member_id = Column(String, ForeignKey('user.user_id', ondelete="CASCADE", onupdate="CASCADE"), nullable=False)
# timestamp at which the share graph event occured
share_time = Column(TIMESTAMP, nullable = False)
# Boolean value to track if notifications is read or not.
# if True then the notification is active, i.e not read
event_active = Column(Boolean, nullable=False)
Copy link
Collaborator

Choose a reason for hiding this comment

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

call it is_active

# We use ForeignKeyConstraint for graph_id and owner_id
# because this is the only to define a composite foreign key
__table_args__ = (
UniqueConstraint('graph_id', 'owner_id', 'group_id', 'member_id'),
ForeignKeyConstraint([graph_id, owner_id], [Graph.graph_id, Graph.user_id], ondelete="CASCADE", onupdate="CASCADE"),
)

#Create indices
Index('graph_public_idx', Graph.public)
Index('graph_owner_idx', Graph.user_id)
Expand Down
5 changes: 4 additions & 1 deletion graphs/templates/base.html
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,10 @@ <h4 class="modal-title" id="myModalLabel">Forgot Information</h4>
{% endif %}
<li><a href="{% url 'features' %}">Features</a></li>
<li><a href="{% url 'help_tutorial' %}">Help</a></li>
<li><a href="{% url 'help_about' %}" style="text-align:center">About Us</a></li>
<li><a href="{% url 'help_about' %}" style="text-align:center">About Us</a></li>
{% if uid != None %}
Copy link
Member

Choose a reason for hiding this comment

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

Is this check sufficient? Should we not check that the user is indeed logged in and is a "valid" user? I am asking without knowledge of how this check is performed in other parts of GraphSpace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this will be enough as uid gets a value only if the user is logged in and a valid user. Logic of this https://github.com/Murali-group/GraphSpace/blob/master/graphs/auth/login.py#L44

Copy link
Contributor Author

Choose a reason for hiding this comment

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

screen shot 2016-07-24 at 4 52 03 pm
The user will get a screen like this if its not a valid log in

<li><a href="{% url 'notifications' %}">Notifications</a></li>
{% endif %}
</ul>

<!-- display login form if not logged in -->
Expand Down
41 changes: 41 additions & 0 deletions graphs/templates/graphs/notifications.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
{% extends 'base.html' %}
{% block content %}
{% load staticfiles %}

{% if uid != None %}

<div class="panel panel-primary">
<div class="panel-heading">
<h3 class="panel-title">Group1</h3>
</div>
<table class="table">
<tr>
<td> <a href="">[email protected] shared graph some_name_of_graph with the group Group 1.</td>
Copy link
Member

@tmmurali tmmurali Jul 24, 2016

Choose a reason for hiding this comment

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

Shouldn't there be generic variables here instead of [email protected], the name of the graph, and the name of the group? After reading the rest of the PR, I understand now that you do not have the full implementation and that most of the methods are stubs.

</tr>
<tr>
<td> <a href="">[email protected] shared graph some_other_graph with the group Group 1.</td>
</tr>

</table>
</div>

<div class="panel panel-primary">
<div class="panel-heading">
<h3 class="panel-title">Group 2</h3>
</div>
<table class="table">
<tr>
<td> <a href="">[email protected] shared graph new_graph with the group Group 2.</td>
</tr>
<tr>
<td> <a href="">[email protected] shared graph test_graph with the group Group 2.</td>
</tr>
<tr>
<td> <a href="">[email protected] shared graph random_graph with the group Group 2.</td>
</tr>

</table>

</div>
{% endif %}
{% endblock %}
3 changes: 3 additions & 0 deletions graphs/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@
url(r'^graphs/public/$', views.public_graphs, name='public_graphs'),
url(r'^graphs/upload/$', views.upload_graph_through_ui, name='upload_graph_through_ui'),

# notifications page
url(r'^notifications/$', views.notifications, name='notifications'),

# view graph page. This contains regular expression to catch url in the form of the following:
# /graphs/email_address/graph_id/
# regex from http://www.regular-expressions.info/email.html
Expand Down
113 changes: 112 additions & 1 deletion graphs/util/db.py
Original file line number Diff line number Diff line change
Expand Up @@ -3648,7 +3648,9 @@ def share_graph_with_group(owner, graph, groupId, groupOwner):
# If they're an owner or a group member, they can add graph to the group
if group_owner != None or group_member != None:
new_shared_graph = models.GroupToGraph(group_id = groupId, group_owner = groupOwner, user_id = owner, graph_id = graph, modified = graph_exists.modified)

members = get_group_members(groupOwner, groupId)
for member in members:
add_share_graph_event(graph, owner, groupId, member.user_id)
db_session.add(new_shared_graph)
db_session.commit()
else:
Expand Down Expand Up @@ -4742,3 +4744,112 @@ def constructResponse(statusCode, message, error):
response['Error'] = error

return response

Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a comment to document the return value for each method.


def add_share_graph_event(graph_id, owner_id, group_id, member_id):
'''
Add a new share graph event to the table.
After sharing the graph with a group this function will create
a share graph event for all the users in that group

@param graph_id: id of the graph shared
@param owner_id: owner of the graph which is shared
@param group_id: id of the grop
@param member_id: id of the member the graph is shared
'''
# Create database connection
db_session = data_connection.new_session()

# Get the current time
cur_time = datetime.now()
Copy link
Collaborator

Choose a reason for hiding this comment

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

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


new_event = models.ShareGraphEvent(graph_id=graph_id, owner_id=owner_id, group_id=group_id, member_id=member_id, share_time=cur_time, event_active=True)
db_session.add(new_event)
db_session.commit()
db_session.close()


def update_share_graph_event(event_id, active, member_id):
Copy link
Collaborator

@adbharadwaj adbharadwaj Jul 29, 2016

Choose a reason for hiding this comment

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

Lets make this method more generic. Keep the signature as

update_share_graph_event(event_id, updated_share_graph_event):

And you can write the code as following:

event = db_session.query(models.ShareGraphEvent).filter(models.ShareGraphEvent.id == event_id)
for (key, value) in updated_share_graph_event.items():
            setattr(event, key, value)     

'''
Update the share graph event. Change its active state.
If active is True then the notification is not read/clicked.

@param event_id: id of the share graph event
@param active: Boolean value, update the state of event
@param member_id: id of the user, the logged in user.
'''
db_session = data_connection.new_session()

try:
event = db_session.query(models.ShareGraphEvent).filter(models.ShareGraphEvent.id == event_id).filter(models.ShareGraphEvent.member_id == member_id).one()
event.event_active = active
db_session.commit()
db_session.close()
except NoResultFound:
db_session.close()
return 'Event not found'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Raise an error instead of returning strings.

There is way of documenting raised errors in python docstring. Please add that as well. The same goes with all methods. A method should return a single datatype or raise an error.



def delete_share_graph_event(event_id, member_id):
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove the member id from this. event id is unique to a share graph event.

'''
Delete the share graph event from the table for the member

@param event_id: id of the share graph event
@param member_id: id of the member

'''
db_session = data_connection.new_session()
try:
event = db_session.query(models.ShareGraphEvent).filter(models.ShareGraphEvent.id == event_id).filter(models.ShareGraphEvent.member_id == member_id).one()
db_session.delete(event)
db_session.commit()
db_session.close()
except NoResultFound:
db_session.close()
return 'Event not found'


def get_share_graph_event_by_member_id(member_id):
'''
Return all the share graph events for a user
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should say returns list of all the share graph events instead of all the share graph events.


@param member_id: id of the user
'''
# Create database connection
db_session = data_connection.new_session()

try:
events = db_session.query(models.ShareGraphEvent).filter(models.ShareGraphEvent.member_id == member_id).all()
db_session.close()
return events
except NoResultFound:
db_session.close()
return None
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should return an empty string. If a method returns a list, it should always return a list.



def get_share_graph_event_by_id(event_id, member_id):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again no need for member id.

'''
Return share graph event notification

@param event_id: id of the event
@param member_id: id of the logged in user
'''
db_session = data_connection.new_session()
try:
event = db_session.query(models.ShareGraphEvent).filter(models.ShareGraphEvent.id == event_id).filter(models.ShareGraphEvent.member_id == member_id).one()
db_session.close()
return event
except NoResultFound:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Document this error and catch this exception in controller.

db_session.close()
return 'Event not found'


def get_all_share_graph_event():
'''
Return all the share graph events.
'''
db_session = data_connection.new_session()
events = db_session.query(models.ShareGraphEvent).all()
db_session.close()
return events

Copy link
Collaborator

Choose a reason for hiding this comment

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

Add another method called set_share_graph_events_inactive(event_ids).

events: list of event_ids

This function should set all events as inactive. Return type should be None. Raise an error if query fails.

Try to make it a single db query. You can use the update api in sqlalchemy.

12 changes: 12 additions & 0 deletions graphs/util/db_test.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
from graphs.models import share_graph_event
from django.test import TestCase
from db import (get_all_share_graph_event, get_share_graph_event_by_id,
get_share_graph_event_by_member_id, delete_share_graph_event,
update_share_graph_event, add_share_graph_event)


class share_graph_event_test(TestCase):
def basic_test(self):
share_event = add_share_graph_event()
# add tests after adding logic

12 changes: 12 additions & 0 deletions graphs/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,18 @@ def _graphs_page(request, view_type):

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

def notifications(request):
# context of the view to be passed in for rendering
context = {}
# handle login
context = login(request)
# Checks to see if a user is currently logged on
uid = request.session['uid']
if uid is None:
context['Error'] = "Please log in to view notifications."
return render(request, 'graphs/error.html', context)
return render(request, 'graphs/notifications.html', context)

def upload_graph_through_ui(request):

if request.method == 'POST':
Expand Down