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

[WIP] Notifications app #2967

Closed
wants to merge 48 commits into from
Closed

[WIP] Notifications app #2967

wants to merge 48 commits into from

Conversation

octavioamu
Copy link
Contributor

@octavioamu octavioamu commented Nov 27, 2018

Description
Checklist
  • linter status: 100% pass
  • changes don't break existing behavior
  • commit message follows commit guidelines
Affected core subsystem(s)
Testing
Refers/Fixes

Refs: #2404

@octavioamu octavioamu self-assigned this Nov 27, 2018
@@ -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.

F401 'django.contrib.admin' imported but unused

@@ -0,0 +1,3 @@
from django.db import models

Choose a reason for hiding this comment

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

F401 'django.db.models' 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.

F401 'django.test.TestCase' imported but unused

@@ -0,0 +1,3 @@
from django.shortcuts import render

Choose a reason for hiding this comment

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

F401 'django.shortcuts.render' imported but unused

from django.apps import AppConfig


class NotificationsConfig(AppConfig):
Copy link
Contributor

Choose a reason for hiding this comment

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

Mind switching this over to singular?

@codecov
Copy link

codecov bot commented Nov 28, 2018

Codecov Report

Merging #2967 into master will decrease coverage by 0.02%.
The diff coverage is 26.92%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2967      +/-   ##
==========================================
- Coverage    29.8%   29.77%   -0.03%     
==========================================
  Files         180      185       +5     
  Lines       13718    13848     +130     
  Branches     1838     1856      +18     
==========================================
+ Hits         4088     4123      +35     
- Misses       9492     9587      +95     
  Partials      138      138
Impacted Files Coverage Δ
app/app/settings.py 80.07% <ø> (ø) ⬆️
app/inbox/utils.py 0% <0%> (ø)
app/inbox/apps.py 0% <0%> (ø)
app/inbox/admin.py 100% <100%> (ø)
app/app/urls.py 87.5% <100%> (+0.26%) ⬆️
app/inbox/views.py 16% <16%> (ø)
app/inbox/models.py 83.33% <83.33%> (ø)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d88630a...a01df33. Read the comment docs.

@mbeacom mbeacom added the wip label Nov 28, 2018
@@ -138,7 +138,7 @@

TEMPLATES = [{
'BACKEND': 'django.template.backends.django.DjangoTemplates',
'DIRS': ['retail/templates/', 'external_bounties/templates/', 'dataviz/templates', 'kudos/templates'],
'DIRS': ['retail/templates/', 'external_bounties/templates/', 'dataviz/templates', 'kudos/templates', 'inbox/templates'],

Choose a reason for hiding this comment

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

E501 line too long (125 > 120 characters)

@octavioamu
Copy link
Contributor Author

@Anish-Agnihotri here is the PR to work with I will be updating it so we can work together. Ping me if anything.


# Create your models here.

class Notification(models.Model):

Choose a reason for hiding this comment

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

E302 expected 2 blank lines, found 1


# Create your views here.

def inbox(request, notification):

Choose a reason for hiding this comment

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

E302 expected 2 blank lines, found 1

@octavioamu
Copy link
Contributor Author

Hey @Anish-Agnihotri any update on this?


# Create your models here.

class Notification(SuperModel):

Choose a reason for hiding this comment

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

E302 expected 2 blank lines, found 1

@@ -0,0 +1,49 @@
from django.contrib.staticfiles.templatetags.staticfiles import static
from django.core.paginator import Paginator

Choose a reason for hiding this comment

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

F401 'django.core.paginator.Paginator' imported but unused

from django.core.paginator import Paginator
from django.shortcuts import render
from django.template.response import TemplateResponse
from django.utils import timezone

Choose a reason for hiding this comment

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

F401 'django.utils.timezone' imported but unused


from django.http import JsonResponse, HttpResponseForbidden

def notifications(request):

Choose a reason for hiding this comment

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

E302 expected 2 blank lines, found 1

def notifications(request):
"""Handle grants explorer."""
profile = request.user.profile if request.user.is_authenticated and request.user.profile else None
limit = request.GET.get('limit', 25)

Choose a reason for hiding this comment

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

F841 local variable 'limit' is assigned to but never used

"""Handle grants explorer."""
profile = request.user.profile if request.user.is_authenticated and request.user.profile else None
limit = request.GET.get('limit', 25)
page = request.GET.get('page', 1)

Choose a reason for hiding this comment

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

F841 local variable 'page' is assigned to but never used

profile = request.user.profile if request.user.is_authenticated and request.user.profile else None
limit = request.GET.get('limit', 25)
page = request.GET.get('page', 1)
sort = request.GET.get('sort_option', '-created_on')

Choose a reason for hiding this comment

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

F841 local variable 'sort' is assigned to but never used


return JsonResponse(params, status=200, safe=False)

def inbox(request):

Choose a reason for hiding this comment

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

E302 expected 2 blank lines, found 1

@@ -0,0 +1,4 @@
from django.apps import AppConfig

class NotificationConfig(AppConfig):

Choose a reason for hiding this comment

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

E302 expected 2 blank lines, found 1

notifications.push(element);
});

markAsRead(newNotifications)

Choose a reason for hiding this comment

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

Missing semicolon. (semi)

}

function markAsRead(notificationsA) {
console.log(notificationsA)

Choose a reason for hiding this comment

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

Unexpected console statement. (no-console)
Missing semicolon. (semi)


function markAsRead(notificationsA) {
console.log(notificationsA)
var notificationRead = parseInt(sessionStorage.getItem('notificationRead'))

Choose a reason for hiding this comment

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

Missing semicolon. (semi)

var notificationRead = parseInt(sessionStorage.getItem('notificationRead'))

if (notificationRead) {
console.log('ping')

Choose a reason for hiding this comment

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

Unexpected console statement. (no-console)
Missing semicolon. (semi)

if (notificationRead) {
console.log('ping')
sessionStorage.removeItem('notificationRead');
console.log('api request readed', notificationRead);

Choose a reason for hiding this comment

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

Unexpected console statement. (no-console)

// notificationRead = parseInt(notificationRead)

const resulta = notificationsA.findIndex(item => {
return item.id === notificationRead

Choose a reason for hiding this comment

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

Missing semicolon. (semi)


const resulta = notificationsA.findIndex(item => {
return item.id === notificationRead
})

Choose a reason for hiding this comment

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

Missing semicolon. (semi)


$(document).ready(function() {

$('.notifications__box').on('click', '[data-notification]', function(e){

Choose a reason for hiding this comment

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

Missing space before opening brace. (space-before-blocks)


$('.notifications__box').on('click', '[data-notification]', function(e){
window.sessionStorage.setItem('notificationRead', e.currentTarget.dataset.notification);
})

Choose a reason for hiding this comment

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

Missing semicolon. (semi)

window.sessionStorage.setItem('notificationRead', e.currentTarget.dataset.notification);
})

})

Choose a reason for hiding this comment

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

Missing semicolon. (semi)

const container = $('.notifications__list');

function requestNotifications() {
console.log(page)

Choose a reason for hiding this comment

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

Unexpected console statement. (no-console)
Missing semicolon. (semi)


$.when(getNotifications).then(function(response) {
// var loadTmp = response.data.length && response.data[0].id !== notifications[0].id;
var loadTmp = response.data.length !== notifications.length

Choose a reason for hiding this comment

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

Expected blank line after variable declarations. (newline-after-var)
Missing semicolon. (semi)

if (loadTmp) {
newNotifications = newData(response.data, notifications);

page = response.has_next ? page+1 : page = 1

Choose a reason for hiding this comment

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

Infix operators must be spaced. (space-infix-ops)
Missing semicolon. (semi)

newNotifications = newData(response.data, notifications);

page = response.has_next ? page+1 : page = 1
console.log(page)

Choose a reason for hiding this comment

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

Unexpected console statement. (no-console)
Missing semicolon. (semi)

</li>`).join(' ')}`;

// if (newItems) {
container.prepend(tmp);

Choose a reason for hiding this comment

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

Expected indentation of 2 spaces but found 4. (indent)

var array = [];
var observedArray = new Proxy(notifications, {
set: function (target, propertyKey, value, receiver) {
console.log(propertyKey+'='+value);

Choose a reason for hiding this comment

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

Expected indentation of 4 spaces but found 8. (indent)
Unexpected console statement. (no-console)
Infix operators must be spaced. (space-infix-ops)

var observedArray = new Proxy(notifications, {
set: function (target, propertyKey, value, receiver) {
console.log(propertyKey+'='+value);
console.log(target,propertyKey, value )

Choose a reason for hiding this comment

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

Expected indentation of 4 spaces but found 8. (indent)
Unexpected console statement. (no-console)
A space is required after ','. (comma-spacing)
There should be no spaces inside this paren. (space-in-parens)
Missing semicolon. (semi)

set: function (target, propertyKey, value, receiver) {
console.log(propertyKey+'='+value);
console.log(target,propertyKey, value )
target[propertyKey] = value;

Choose a reason for hiding this comment

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

Expected indentation of 4 spaces but found 8. (indent)

console.log(propertyKey+'='+value);
console.log(target,propertyKey, value )
target[propertyKey] = value;
return true

Choose a reason for hiding this comment

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

Expected indentation of 4 spaces but found 8. (indent)
Missing semicolon. (semi)

console.log(target,propertyKey, value )
target[propertyKey] = value;
return true
}

Choose a reason for hiding this comment

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

Expected indentation of 2 spaces but found 4. (indent)

if 'read' in req_body:
for i in req_body['read']:
entry = Notification.objects.filter(id=i)
#if entry.to_user_id.id == request.user.id and len(entry) != 0:

Choose a reason for hiding this comment

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

E265 block comment should start with '# '

@owocki
Copy link
Contributor

owocki commented Dec 12, 2018

performance concerns:

  • do we do DB queries on every page
  • will the DB queries be super slow when inbox table grows (activity feed table size, this is slow)? do we have approprirate indexes?
  • can we turn off longpolling?
  • do longpolling/websockets take down gunicorn? can we move these requests to their own server?

@octavioamu
Copy link
Contributor Author

Moved to #3221 without longpulling and vue.js as discussed

@octavioamu octavioamu closed this Dec 16, 2018
@thelostone-mc thelostone-mc deleted the new-notifications branch December 28, 2018 20:11
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.

4 participants