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

User favorites route #38

Merged
merged 4 commits into from
Jan 20, 2017
Merged

User favorites route #38

merged 4 commits into from
Jan 20, 2017

Conversation

alanmoo
Copy link
Contributor

@alanmoo alanmoo commented Jan 19, 2017

No description provided.

@alanmoo alanmoo requested a review from Pomax January 19, 2017 20:38
@@ -117,6 +117,17 @@ class EntryView(RetrieveAPIView):
pagination_class = None


class FavoriteEntries(ListAPIView):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't inherit from EntriesListView because I didn't want this to be POSTable, but we could use some of the filtering options that view has. Thoughts on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, could we implement this as, itself, a filter? I.e. when users call /entries/favorites[?args] Django internally serves up the result for /entries/[?args]&is_favorited=True?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not easily, as per our chat.

def get_queryset(self):
entries = set()
user = self.request.user.id
for f in UserFavorites.objects.filter(user=user).select_related('entry'):
Copy link
Contributor

Choose a reason for hiding this comment

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

small nit: the interpreter doesn't care, but maintainers do, so avoiding single letter variables is generally good practice. Can we call this what it is? (like "entry" or "favorite", although with the user_favs refactor, "bookmark")

@@ -117,6 +117,17 @@ class EntryView(RetrieveAPIView):
pagination_class = None


class FavoriteEntries(ListAPIView):
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, could we implement this as, itself, a filter? I.e. when users call /entries/favorites[?args] Django internally serves up the result for /entries/[?args]&is_favorited=True?

Copy link
Contributor

@Pomax Pomax left a comment

Choose a reason for hiding this comment

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

preeeeetty nice!

@alanmoo
Copy link
Contributor Author

alanmoo commented Jan 19, 2017

All this needs is tests then?

@alanmoo
Copy link
Contributor Author

alanmoo commented Jan 19, 2017

I've added some tests. One more look at this?

self.assertEqual(bookmarkResponse.status_code, 200)

bookmarkJson = json.loads(str(bookmarkResponse.content, 'utf-8'))
self.assertEqual(len(bookmarkJson), 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

missing newline because github is hilarious that way

Copy link
Contributor

@Pomax Pomax left a comment

Choose a reason for hiding this comment

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

R+ with a nit because github wants newlines at the end of files for... reasons...

@alanmoo alanmoo merged commit e030e6e into user_favs Jan 20, 2017
@alanmoo alanmoo deleted the favs-view branch January 20, 2017 00:35
alanmoo pushed a commit that referenced this pull request Jan 20, 2017
* user/entry linking through a "favorite" construction.

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 (#38)

* User favorites route

* ‘favorite’ -> ‘bookmark’

* Bookmark view tests
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.

None yet

2 participants