User bookmarking #14

Pomax · 2017-01-13T00:17:27Z

/entries and /entries/1234 show entries/a single entry with their total fave count as field favorite_count, and a field is_favorite that is true for a user that is a) logged in, and b) has favorited this entry. In any other case, it will be false.
/entries/123/favorite toggles a fave/not-fave value for entry 123 for the currently logged in user. If a user is not logged in, they will receive a 403

fixes #12
fixes #30

alanmoo · 2017-01-13T21:31:54Z

pulseapi/users/test_views.py

@@ -0,0 +1,12 @@
+from django.core.urlresolvers import reverse


Is this file old?

I think so, I'll remove it.

alanmoo · 2017-01-13T21:32:14Z

pulseapi/users/serializers.py

+    Serializes a {user,entry,when} fave.
+    """
+
+    user = '__all__'


What's this do?

good question @gideonthomas do you remember why we needed this? If so, we can document it in the source

Actually, I don't know what this is :P I thought all serializer fields should be instances. Are you sure this is working?

__all__ normally exposes all public methods for use in import ... from ... statements including those prefixed with underscores (usually 'hidden') but in this case it doesn't look like it actually does anything, so I'll remove it.

alanmoo · 2017-01-13T21:41:43Z

pulseapi/users/admin.py

    def entries(self, instance):
        """
        Show all entries as a string of titles. In the future we should make them links.
        """
+        print('entries instance')


Remove this logging

alanmoo · 2017-01-13T22:01:31Z

pulseapi/entries/views.py

 from rest_framework.generics import ListCreateAPIView, RetrieveAPIView
 from rest_framework.pagination import PageNumberPagination
 from rest_framework.response import Response

 from pulseapi.entries.models import Entry
 from pulseapi.entries.serializers import EntrySerializer
+from pulseapi.users.models import UserFavorites
+
+# FIXME: TODO: Remove the handling for GET, so that only PUT works


Is the GET there just so it's easier to test?

correct. The "real" use would be PUT only, but for testing a GET is way easier to do without writing an XHR test page

alanmoo · 2017-01-13T22:07:11Z

pulseapi/entries/views.py

+
+# FIXME: TODO: Remove the handling for GET, so that only PUT works
+@api_view(['GET','PUT'])
+def toggle_favorite(request, entryid):


Just so I understand what's going on here because I've seen this before, can you explain line 17 and 18 from a django standpoint? Why is this a function and not a view class? Is 17 the indicator that it's a generic GET/PUT view which does nothing special?

line 17 says "only expose this def when the http verb is GET or PUT (http verb whitelist)

line 18 is a function that services a view (see urls.py) as a general function rather than a class entry, as faving an entry is technically something "outside" of entries and users.

alanmoo

See above comment

had to 'dismiss' to get rid of a detached review comment

Pomax · 2017-01-16T20:56:51Z

pulseapi/creators/tests.py

@@ -2,26 +2,63 @@

 from django.test import TestCase
 from django.test import Client
-from pulseapi.tests import PulseTestCase


Did my PR reset this to older code?

Pomax · 2017-01-16T21:02:29Z

pulseapi/entries/test_views.py

@@ -1,9 +1,8 @@
 import json

-from django.core.urlresolvers import reverse


hm, this looks to have old test code, too.

Pomax · 2017-01-16T21:10:00Z

pulseapi/entries/views.py

@@ -21,8 +56,8 @@ def post_validate(request):
    nonce = False

    if request.data:
-        csrf_token = request.data['csrfmiddlewaretoken']
-        nonce = request.data['nonce']
+        csrf_token = request.data.get('csrfmiddlewaretoken', False)


this was throwing key errors so I turned it into get rather than direct access to get a fallback value.

REST endpoint: <PUT> /entries/<id>/bookmark Behaviour: calling the endpoint, by an authenticated user, will mark the entry as bookmarked by the that user. Calling it again unbookmarks the entry.

* User favorites route * ‘favorite’ -> ‘bookmark’ * Bookmark view tests

alanmoo · 2017-01-20T14:57:48Z

pulseapi/entries/serializers.py

+
+    is_bookmarked = serializers.SerializerMethodField()
+
+    def get_is_bookmarked(self, instance):


This is the kind of magic that bothers me. I understand it, but it still feels weird.

I hate Django for this kind of thing.

alanmoo · 2017-01-20T15:53:02Z

pulseapi/entries/views.py

+from pulseapi.users.models import UserBookmarks
+
+@api_view(['PUT'])
+def toggle_bookmark(request, entryid):


We're sticking with the toggling behavior?

After thinking about it some more, entries already say whether they are bookmarked or not when you call /entries so for regular operation we don't need a "toggle yes or no", the only time we would need that is for bulk bookmark syncing, which we should probably do on its own dedicated route so a full set of bookmarks can be sent over in a single call.

That makes sense. There's then a little bit of manual state management between the front and back end, but it seems reasonable.

alanmoo · 2017-01-20T16:04:27Z

pulseapi/creators/migrations/0001_initial.py

@@ -21,7 +18,6 @@ class Migration(migrations.Migration):
            fields=[
                ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
                ('name', models.CharField(max_length=140)),
-                ('user', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, related_name='as_creator', to=settings.AUTH_USER_MODEL)),


I know I probably shouldn't be reading into the migrations files too much but why is this being removed?

Should we merge these and then follow up with an immediate rebuild of migrations?

that would be my suggestion too, yes. It's time to wipe the compound migrations we've been accumulating (except for issues, which is one step and has data) and rebuild everything as single step migrations.

alanmoo

This looks good, just a few clarifications before merging?

Pomax assigned alanmoo Jan 13, 2017

Pomax requested a review from alanmoo January 13, 2017 21:25

alanmoo reviewed Jan 13, 2017

View reviewed changes

alanmoo previously requested changes Jan 13, 2017

View reviewed changes

Pomax force-pushed the user_favs branch from 0ba1a16 to f84e431 Compare January 16, 2017 17:47

Pomax changed the title ~~[WIP] User favoriting~~ User favoriting Jan 16, 2017

Pomax force-pushed the user_favs branch from 0668099 to 9f89637 Compare January 16, 2017 20:53

Pomax commented Jan 16, 2017

View reviewed changes

Pomax force-pushed the user_favs branch from c4b833a to 5ed9b16 Compare January 16, 2017 21:01

Pomax commented Jan 16, 2017

View reviewed changes

Pomax force-pushed the user_favs branch from cc75c62 to 65d0eec Compare January 16, 2017 21:08

Pomax commented Jan 16, 2017

View reviewed changes

Pomax force-pushed the user_favs branch from 65d0eec to e595a19 Compare January 17, 2017 20:36

user/entry linking through a "favorite" construction.

af78c9e

REST endpoint: <PUT> /entries/<id>/bookmark Behaviour: calling the endpoint, by an authenticated user, will mark the entry as bookmarked by the that user. Calling it again unbookmarks the entry.

Pomax force-pushed the user_favs branch from f75a2b2 to af78c9e Compare January 19, 2017 21:44

Pomax changed the title ~~User favoriting~~ User bookmarking Jan 19, 2017

Pomax requested a review from alanmoo January 19, 2017 22:03

User favorites route (#38)

e030e6e

* User favorites route * ‘favorite’ -> ‘bookmark’ * Bookmark view tests

alanmoo reviewed Jan 20, 2017

View reviewed changes

alanmoo approved these changes Jan 20, 2017

View reviewed changes

alanmoo merged commit 5f8cbfd into master Jan 20, 2017

alanmoo deleted the user_favs branch January 20, 2017 17:04

alanmoo mentioned this pull request Mar 1, 2017

Split bookmark and unbookmark routes #94

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

User bookmarking #14

User bookmarking #14

Pomax commented Jan 13, 2017 •

edited

Loading

alanmoo Jan 13, 2017

Pomax Jan 13, 2017

alanmoo Jan 13, 2017

Pomax Jan 13, 2017

gideonthomas Jan 16, 2017

Pomax Jan 16, 2017

alanmoo Jan 13, 2017

alanmoo Jan 13, 2017

Pomax Jan 13, 2017

alanmoo Jan 13, 2017

Pomax Jan 13, 2017

alanmoo left a comment

Pomax Jan 16, 2017

Pomax Jan 16, 2017

Pomax Jan 16, 2017

alanmoo Jan 20, 2017

Pomax Jan 20, 2017

alanmoo Jan 20, 2017

Pomax Jan 20, 2017

alanmoo Jan 20, 2017

alanmoo Jan 20, 2017

Pomax Jan 20, 2017

alanmoo left a comment

		@@ -0,0 +1,12 @@
		from django.core.urlresolvers import reverse

		@@ -1,9 +1,8 @@
		import json

		from django.core.urlresolvers import reverse


		is_bookmarked = serializers.SerializerMethodField()

		def get_is_bookmarked(self, instance):

User bookmarking #14

User bookmarking #14

Conversation

Pomax commented Jan 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanmoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanmoo left a comment

Choose a reason for hiding this comment

Pomax commented Jan 13, 2017 •

edited

Loading