fix: online status middleware cache #955

MMFernando · 2019-05-07T15:58:24Z

Replaced existing Django Online Status Middleware with the one cloned here . Integrated the code with the project and existing tests.

The code that was there before seemed to be failing to remove users from the online user list, causing an error as the cache was set to hold more than its data size limit of 1 MB.

Documentation was added.

mrniket · 2019-05-07T15:58:36Z

This change is

mrniket

Reviewed 10 of 93 files at r1.
Reviewable status: 10 of 93 files reviewed, 8 unresolved discussions (waiting on @mariaFernando)

portal/autoconfig.py, line 107 at r1 (raw file):

        "django.contrib.auth.middleware.AuthenticationMiddleware",
        "django.contrib.auth.middleware.SessionAuthenticationMiddleware",
        "portal.middleware.django_online_status.online_status.middleware.OnlineStatusMiddleware",

the folder structure here seems a bit redundant.... portal -> middleware -> .... -> middleware -> ...

as a note, if this changes, you need to make this change in deploy-appengine possibly also

portal/middleware/django_online_status/.hgignore, line 2 at r1 (raw file):

syntax: glob

we don't use mercurial so we don't need this here

portal/middleware/django_online_status/.hgtags, line 1 at r1 (raw file):

c14b815efd477f233cf57c7605c14773ab6f12d5 0_1_0

we don't use mercurial, delete this

portal/middleware/django_online_status/AUTHORS, line 1 at r1 (raw file):

Jakub Zalewski http://zalew.tumblr.com

delete this file

portal/middleware/django_online_status/README, line 1 at r1 (raw file):

========================

delete this

portal/middleware/django_online_status/setup.py, line 1 at r1 (raw file):

#!/usr/bin/env python

this doesn't need to be its own package, delete this

portal/middleware/django_online_status/UNLICENSE, line 1 at r1 (raw file):

This is free and unencumbered software released into the public domain.

delete this

portal/middleware/django_online_status/.hg/00changelog.i, line 0 at r1 (raw file):
delete the .hg folder please

mrniket

Reviewed 2 of 93 files at r1.
Reviewable status: 12 of 93 files reviewed, 13 unresolved discussions (waiting on @mariaFernando)

a discussion (no related file):
Can we have a more detailed description of the fix please? The middleware was already there, this PR should describe what was fixed in it

portal/middleware/django_online_status/online_status/middleware.py, line 15 at r1 (raw file):

class OnlineStatusMiddleware(MiddlewareMixin):

to make this a bit more futureproof, can we use the modern API for middleware here please?

portal/middleware/django_online_status/online_status/middleware.py, line 15 at r1 (raw file):

class OnlineStatusMiddleware(MiddlewareMixin):

Can we confirm that the behavioural differences between MIDDLEWARE and MIDDLEWARE_CLASSES do not affect the way this middleware works?

portal/middleware/django_online_status/online_status/models.py, line 0 at r1 (raw file):
afaik middleware do not need a models file, I would remove this

portal/middleware/django_online_status/online_status/status.py, line 96 at r1 (raw file):

def status_for_user(user):
    """Return status for user, duh?"""

Let's replace this with a more useful docstring

mrniket

Reviewable status: 12 of 93 files reviewed, 14 unresolved discussions (waiting on @mariaFernando)

a discussion (no related file):
@dent50cent are your docs changes in this PR?

MMFernando · 2019-05-08T08:29:08Z

portal/middleware/django_online_status/.hgignore, line 2 at r1 (raw file):

Previously, mrniket (Niket Shah) wrote…

we don't use mercurial so we don't need this here

Done.

MMFernando · 2019-05-08T08:29:11Z

portal/middleware/django_online_status/.hgtags, line 1 at r1 (raw file):

Previously, mrniket (Niket Shah) wrote…

we don't use mercurial, delete this

Done.

MMFernando · 2019-05-08T08:29:26Z

portal/middleware/django_online_status/AUTHORS, line 1 at r1 (raw file):

Previously, mrniket (Niket Shah) wrote…

delete this file

Done.

MMFernando · 2019-05-08T08:29:56Z

portal/middleware/django_online_status/README, line 1 at r1 (raw file):

Previously, mrniket (Niket Shah) wrote…

delete this

Done.

MMFernando · 2019-05-08T08:31:52Z

portal/middleware/django_online_status/setup.py, line 1 at r1 (raw file):

Previously, mrniket (Niket Shah) wrote…

this doesn't need to be its own package, delete this

Done.

MMFernando · 2019-05-08T08:32:06Z

portal/middleware/django_online_status/UNLICENSE, line 1 at r1 (raw file):

Previously, mrniket (Niket Shah) wrote…

delete this

Done.

MMFernando · 2019-05-08T08:36:36Z

portal/middleware/django_online_status/online_status/models.py, line at r1 (raw file):

Previously, mrniket (Niket Shah) wrote…

afaik middleware do not need a models file, I would remove this

Done.

MMFernando · 2019-05-08T08:38:17Z

portal/middleware/django_online_status/online_status/status.py, line 96 at r1 (raw file):

Previously, mrniket (Niket Shah) wrote…

Let's replace this with a more useful docstring

Done.

mrniket

Reviewable status: 12 of 94 files reviewed, 14 unresolved discussions (waiting on @mariaFernando and @mrniket)

portal/middleware/django_online_status/.hgignore, line 2 at r1 (raw file):

Previously, mariaFernando (Maria Mafalda Fernando) wrote…

Done.

not done? perhaps you haven't pushed?

mrniket

Reviewable status: 12 of 94 files reviewed, 14 unresolved discussions (waiting on @mariaFernando and @mrniket)

portal/middleware/django_online_status/.hgtags, line 1 at r1 (raw file):

Previously, mariaFernando (Maria Mafalda Fernando) wrote…

Done.

not done?

MMFernando · 2019-05-08T10:42:24Z

a discussion (no related file):

Previously, mrniket (Niket Shah) wrote…

Can we have a more detailed description of the fix please? The middleware was already there, this PR should describe what was fixed in it

Is the description more fitting now, or should I go more in detail?

MMFernando · 2019-05-08T10:45:12Z

portal/autoconfig.py, line 107 at r1 (raw file):

Previously, mrniket (Niket Shah) wrote…

the folder structure here seems a bit redundant.... portal -> middleware -> .... -> middleware -> ...

as a note, if this changes, you need to make this change in deploy-appengine possibly also

The folder structure is now portal/middleware/django_online_status. Should I rename middleware.py, too?

MMFernando · 2019-05-08T10:45:36Z

portal/middleware/django_online_status/.hgignore, line 2 at r1 (raw file):

Previously, mrniket (Niket Shah) wrote…

not done? perhaps you haven't pushed?

I hadn't pushed, but it seems like the deletion didn't get properly pushed..?

MMFernando · 2019-05-08T10:51:22Z

portal/middleware/django_online_status/.hg/00changelog.i, line at r1 (raw file):

Previously, mrniket (Niket Shah) wrote…

delete the .hg folder please

Done.

MMFernando · 2019-05-08T10:56:00Z

a discussion (no related file):

Previously, mrniket (Niket Shah) wrote…

@dent50cent are your docs changes in this PR?

They are

mrniket

Reviewed 1 of 2 files at r2.
Reviewable status: 2 of 20 files reviewed, 16 unresolved discussions (waiting on @dent50cent, @mariaFernando, and @mrniket)

docs/online_status_middleware.md, line 9 at r4 (raw file):

There are couple of template tags to access this information to help make use of this functionality.

Also, this middleware contains a couple of view/urls by default, the only one that should exist is the `/test` url as one of the others lists every single user currently online for everyone to see which may pose a security risk, and the other is an example which probably shouldn't exist in a production enviroment, if these are present in your project (or are not ignored) please make sure they don't end up in production.

I think the views and urls won't be there in the end

portal/autoconfig.py, line 107 at r1 (raw file):

Previously, mariaFernando (Maria Mafalda Fernando) wrote…

The folder structure is now portal/middleware/django_online_status. Should I rename middleware.py, too?

well, if you use the __init.py__ in django_online_status to expose the OnlineStatusMiddleware there, you don't need to specify the middleware.py part in the import path

portal/autoconfig.py, line 107 at r1 (raw file):

        "django.contrib.auth.middleware.AuthenticationMiddleware",
        "django.contrib.auth.middleware.SessionAuthenticationMiddleware",
        "portal.middleware.django_online_status.online_status.middleware.OnlineStatusMiddleware",

also, this doesn't make a lot of difference, but we don't need the django_ prefix here if we are already in a django project, right?

mrniket

Reviewed 11 of 93 files at r1, 32 of 38 files at r3, 42 of 45 files at r4.
Reviewable status: 17 of 20 files reviewed, 19 unresolved discussions (waiting on @dent50cent, @mariaFernando, and @mrniket)

portal/middleware/django_online_status/.hg/undo.bookmarks, line 0 at r4 (raw file):
this file looks to still be present?

portal/middleware/django_online_status/.hg/store/phaseroots, line 0 at r4 (raw file):
this file seems to still be present?

portal/middleware/django_online_status/.hg/store/undo.phaseroots, line 0 at r4 (raw file):
this file seems to still be present?

portal/middleware/django_online_status/.hg/wcache/checkisexec, line 0 at r4 (raw file):
this file seems to still be there?

portal/middleware/django_online_status/.hg/wcache/checklink-target, line 0 at r4 (raw file):
this file seems to still be there?

portal/middleware/django_online_status/build/lib/online_status/models.py, line 0 at r4 (raw file):
this file seems to still be there?

portal/middleware/django_online_status/build/lib/online_status/templatetags/init.py, line 0 at r4 (raw file):
this file seems to still be there?

portal/middleware/django_online_status/online_status/templatetags/init.py, line 0 at r4 (raw file):
template tags should probably not be in the middleware folder right?

mrniket

Reviewable status: 17 of 20 files reviewed, 19 unresolved discussions (waiting on @mariaFernando and @mrniket)

portal/tests/test_online_status.py, line 91 at r4 (raw file):

        self.client.get(reverse("dashboard"))
        useronline = cache.get(status.CACHE_PREFIX_USER % self.user1.pk)

maybe it's best to test user2 here as user1 is already testing in setup_conditions?

MMFernando · 2019-05-09T08:22:34Z

portal/middleware/django_online_status/online_status/middleware.py, line 15 at r1 (raw file):

Previously, mrniket (Niket Shah) wrote…

Ah maybe I wasn't clear, I'm concerned and wanted to check about these differences:

We've checked it, it won't affect this middleware in particular

MMFernando

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @mariaFernando and @mrniket)

a discussion (no related file):

Previously, mrniket (Niket Shah) wrote…

just wondering why the idle status was removed?

We came to the conclusion it wasn't needed and is unlikely to be needed in the future for this functionality (if it is, it's not hard to implement again)

portal/autoconfig.py, line 107 at r6 (raw file):

Previously, mrniket (Niket Shah) wrote…

can we have a PR ready to change this in deploy appengine too? https://github.com/ocadotechnology/codeforlife-deploy-appengine/blob/master/django_site/settings.py#L62

Will do

mrniket

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @mariaFernando)

a discussion (no related file):

Previously, mariaFernando (Maria Mafalda Fernando) wrote…

We came to the conclusion it wasn't needed and is unlikely to be needed in the future for this functionality (if it is, it's not hard to implement again)

I don't remember this conversation... normally decisions to remove or add functionality go through the PO

I would add it back, we don't know to impact of that change and it shouldn't be in a bug fix PR. My guess is that it could be useful for teachers monitoring their classes.

MMFernando · 2019-05-09T09:53:26Z

a discussion (no related file):

Previously, mrniket (Niket Shah) wrote…

I don't remember this conversation... normally decisions to remove or add functionality go through the PO

I would add it back, we don't know to impact of that change and it shouldn't be in a bug fix PR. My guess is that it could be useful for teachers monitoring their classes.

You're right, I'll add it back in. What should the offline and idle times be, 5 and 10 minutes like the original functionality?

MMFernando

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @mariaFernando and @mrniket)

portal/tests/test_online_status.py, line 51 at r6 (raw file):

Previously, mrniket (Niket Shah) wrote…

can we make the sleep time as small as possible please

The override wasn't working properly, fixed now but it still seems to need rounded seconds, especially with the idle/offline distinction

portal/tests/test_online_status.py, line 100 at r6 (raw file):

Previously, mrniket (Niket Shah) wrote…

if we manually change user1.seen then we can probably test this without a sleep

the set_active method sets seen to datetime.now() so it can't be manually set

removed offline users from list of online users, which wasn't there before and might have caused the cache overflow. Online, offline and idle tests have been rewritten

…ne_status

MMFernando · 2019-05-09T14:39:22Z

portal/middleware/online_status/status.py, line 92 at r6 (raw file):

Previously, mrniket (Niket Shah) wrote…

you can just return the expression here...

Removed that function

MMFernando · 2019-05-09T14:42:11Z

portal/autoconfig.py, line 107 at r6 (raw file):

Previously, mariaFernando (Maria Mafalda Fernando) wrote…

Will do

Done.

faucomte97

Reviewed 2 of 3 files at r7.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @mrniket)

faucomte97

Dismissed @mrniket from 2 discussions.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mrniket)

portal/autoconfig.py, line 107 at r6 (raw file):

Previously, mariaFernando (Maria Mafalda Fernando) wrote…

Done.

Resolving.

portal/middleware/online_status/status.py, line 92 at r6 (raw file):

Previously, mariaFernando (Maria Mafalda Fernando) wrote…

Removed that function

Resolving.

mrniket

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mariaFernando and @mrniket)

a discussion (no related file):

Previously, mariaFernando (Maria Mafalda Fernando) wrote…

You're right, I'll add it back in. What should the offline and idle times be, 5 and 10 minutes like the original functionality?

sounds good 👍 thanks

mrniket

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mariaFernando)

portal/tests/test_online_status.py, line 100 at r6 (raw file):

Previously, mariaFernando (Maria Mafalda Fernando) wrote…

the set_active method sets seen to datetime.now() so it can't be manually set

there are two ways around this, you can either mock/patch the datetime.now() method for the first request or you can access the user object from the cache (which I believe you do below) and set the seen field on it

mrniket

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @mariaFernando)

portal/tests/test_online_status.py, line 51 at r6 (raw file):

Previously, mariaFernando (Maria Mafalda Fernando) wrote…

The override wasn't working properly, fixed now but it still seems to need rounded seconds, especially with the idle/offline distinction

sorry, what do you mean by rounded seconds?

portal/tests/test_online_status.py, line 48 at r7 (raw file):

from portal.middleware.online_status import status

status.TIME_OFFLINE = 3

having this here will on the module level mean that these settings could be used elsewhere. We need to find a way of doing this only for the TestCase below

MMFernando · 2019-05-10T13:09:04Z

portal/tests/test_online_status.py, line 51 at r6 (raw file):

Previously, mrniket (Niket Shah) wrote…

sorry, what do you mean by rounded seconds?

I used fractional values and it doesn't detect the difference between idle and offline times, possibly because of this line: seconds = (datetime.now() - online_status.seen).seconds . I might be wrong, though

MMFernando · 2019-05-10T13:13:24Z

portal/tests/test_online_status.py, line 100 at r6 (raw file):

Previously, mrniket (Niket Shah) wrote…

there are two ways around this, you can either mock/patch the datetime.now() method for the first request or you can access the user object from the cache (which I believe you do below) and set the seen field on it

Accessing the user object from the cache won't work, since it's the request that simultaneously sets the status and seen to datetime.now(), so setting it manually would always be overriden when the status is set. But I'll look into that first option

MMFernando · 2019-05-10T13:14:22Z

portal/tests/test_online_status.py, line 48 at r7 (raw file):

Previously, mrniket (Niket Shah) wrote…

having this here will on the module level mean that these settings could be used elsewhere. We need to find a way of doing this only for the TestCase below

Do you have any suggestion? override-settings doesn't seem to work. I'll keep looking for a solution and we can discuss it if I can't find one

mrniket

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @mariaFernando and @mrniket)

portal/tests/test_online_status.py, line 48 at r7 (raw file):

Previously, mariaFernando (Maria Mafalda Fernando) wrote…

Do you have any suggestion? override-settings doesn't seem to work. I'll keep looking for a solution and we can discuss it if I can't find one

let's discuss on Monday

mrniket

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @mariaFernando)

portal/tests/test_online_status.py, line 100 at r6 (raw file):

Previously, mariaFernando (Maria Mafalda Fernando) wrote…

Accessing the user object from the cache won't work, since it's the request that simultaneously sets the status and seen to datetime.now(), so setting it manually would always be overriden when the status is set. But I'll look into that first option

👍

no more sleep :D

MMFernando · 2019-05-10T15:32:10Z

a discussion (no related file):

Previously, mrniket (Niket Shah) wrote…

sounds good 👍 thanks

Done.

MMFernando · 2019-05-10T15:33:34Z

portal/tests/test_online_status.py, line 48 at r7 (raw file):

Previously, mrniket (Niket Shah) wrote…

let's discuss on Monday

Used mock patch, seems to work well and the sleep and setting override aren't needed anymore

MMFernando · 2019-05-10T15:33:37Z

portal/tests/test_online_status.py, line 100 at r6 (raw file):

Previously, mrniket (Niket Shah) wrote…

👍

Done.

faucomte97

Reviewed 1 of 3 files at r8.
Reviewable status: 7 of 8 files reviewed, 3 unresolved discussions (waiting on @faucomte97 and @mrniket)

mrniket

Reviewed 1 of 3 files at r8.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mariaFernando)

portal/tests/test_online_status.py, line 48 at r7 (raw file):

Previously, mariaFernando (Maria Mafalda Fernando) wrote…

Used mock patch, seems to work well and the sleep and setting override aren't needed anymore

great 👍

mrniket

Reviewable status: complete! all files reviewed, all discussions resolved

Maria Mafalda Fernando and others added 3 commits May 7, 2019 09:32

replaced django middleware code

a6829ed

created doc to explain django-online-status

827e541

fixed it?

4b72e88

mrniket suggested changes May 8, 2019

View reviewed changes

Merge branch 'create_middleware_docs' into fix_online_status

68d3fa3

mrniket suggested changes May 8, 2019

View reviewed changes

deleted unused files, deleted idle status

1843737

deleted hidden mercurial files, fixed one codeclimate issue

c41f230

Maria Mafalda Fernando added 2 commits May 8, 2019 13:13

fixed code climate issues

90b035d

fixed code climate issue

0c38e69

mrniket suggested changes May 8, 2019

View reviewed changes

MMFernando commented May 9, 2019

View reviewed changes

mrniket suggested changes May 9, 2019

View reviewed changes

MMFernando commented May 9, 2019

View reviewed changes

Maria Mafalda Fernando added 2 commits May 9, 2019 15:10

changed the tests, integrated idle and old working code back in

d15ccd0

removed offline users from list of online users, which wasn't there before and might have caused the cache overflow. Online, offline and idle tests have been rewritten

Merge remote-tracking branch 'origin/fix_online_status' into fix_onli…

e72dcf9

…ne_status

MMFernando changed the title ~~fix: online status~~ fix: online status middleware cache May 9, 2019

code climate fix

e55569f

faucomte97 approved these changes May 10, 2019

View reviewed changes

mrniket suggested changes May 10, 2019

View reviewed changes

changed idle and offline times and mocked time for test

54f63f5

no more sleep :D

faucomte97 reviewed May 10, 2019

View reviewed changes

mrniket suggested changes May 13, 2019

View reviewed changes

mrniket approved these changes May 13, 2019

View reviewed changes

MMFernando merged commit 0ea74e8 into master May 13, 2019

MMFernando deleted the fix_online_status branch May 13, 2019 08:29

fix: online status middleware cache #955

fix: online status middleware cache #955

Conversation

MMFernando commented May 7, 2019 • edited Loading

mrniket commented May 7, 2019

mrniket left a comment

Choose a reason for hiding this comment

mrniket left a comment

Choose a reason for hiding this comment

mrniket left a comment

Choose a reason for hiding this comment

MMFernando commented May 8, 2019

MMFernando commented May 8, 2019

MMFernando commented May 8, 2019

MMFernando commented May 8, 2019

MMFernando commented May 8, 2019

MMFernando commented May 8, 2019

MMFernando commented May 8, 2019

MMFernando commented May 8, 2019

mrniket left a comment

Choose a reason for hiding this comment

mrniket left a comment

Choose a reason for hiding this comment

MMFernando commented May 8, 2019

MMFernando commented May 8, 2019

MMFernando commented May 8, 2019

MMFernando commented May 8, 2019

MMFernando commented May 8, 2019

mrniket left a comment

Choose a reason for hiding this comment

mrniket left a comment

Choose a reason for hiding this comment

mrniket left a comment

Choose a reason for hiding this comment

MMFernando commented May 9, 2019

MMFernando left a comment

Choose a reason for hiding this comment

mrniket left a comment

Choose a reason for hiding this comment

MMFernando commented May 9, 2019

MMFernando left a comment

Choose a reason for hiding this comment

MMFernando commented May 9, 2019

MMFernando commented May 9, 2019

faucomte97 left a comment

Choose a reason for hiding this comment

faucomte97 left a comment

Choose a reason for hiding this comment

mrniket left a comment

Choose a reason for hiding this comment

mrniket left a comment

Choose a reason for hiding this comment

mrniket left a comment

Choose a reason for hiding this comment

MMFernando commented May 10, 2019

MMFernando commented May 10, 2019

MMFernando commented May 10, 2019

mrniket left a comment

Choose a reason for hiding this comment

mrniket left a comment

Choose a reason for hiding this comment

MMFernando commented May 10, 2019

MMFernando commented May 10, 2019

MMFernando commented May 10, 2019

faucomte97 left a comment

Choose a reason for hiding this comment

mrniket left a comment

Choose a reason for hiding this comment

mrniket left a comment

Choose a reason for hiding this comment

MMFernando commented May 7, 2019 •

edited

Loading