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

#163383192 Add functionality to filter and search for articles #37

Merged
merged 1 commit into from
Feb 14, 2019

Conversation

oma0256
Copy link
Contributor

@oma0256 oma0256 commented Feb 13, 2019

What does this PR do?

  • Allow a user filter for articles based on article title, tags and author username.
  • Allow user search using keywords.

How should this be manually tested?

  • Download and install postman onto you're local machine
  • Open postman
  • Then visit the route https://ah-backend-summer-stagin-pr-37.herokuapp.com/api/v1/articles/?<param>=<search_text> <param> can be author, title, tag <search_text> the text you want to search or filter basing on to filter for articles or https://ah-backend-summer-stagin-pr-37.herokuapp.com/api/v1/articles/?search=<search_text> to search for articles using GET.

What are the relevant pivotal tracker stories?
#163383192

@karuhanga karuhanga temporarily deployed to ah-backend-summer-stagin-pr-37 February 13, 2019 12:38 Inactive
Copy link
Contributor

@fahdjamy fahdjamy left a comment

Choose a reason for hiding this comment

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

Work looks good, precise and on point but add a little more comments in your classes plus generally make function names more descriptive and precise. like I commented on certain lines I've added on my review.
Otherwise, great Job done @oma0256.

self.assertEqual(response.status_code, status.HTTP_200_OK)
self.assertEqual(len(response.data), 0)

def test_user_can_filter_by_articles_by_title(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the good work done @oma0256. But think you want to do here is name this method as test_user_can_filter_articles_by_title.
Thanks.

self.assertEqual(len(response.data), 0)

def test_user_can_filter_by_articles_by_tag(self):
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the same thing might apply here. its test_user_can_filter_articles_by_tag.

@@ -84,18 +85,27 @@
# Password validation
# https://docs.djangoproject.com/en/1.11/ref/settings/#auth-password-validators

USER_ATTRIBUTE_SIMILARITY_VALIDATOR = \
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the good work. If you don't mind me asking, what is the functionality of the lines below that were added in this file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@missvicki I touched this file so they were pep8 issues I was just resolving those issues

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright, makes sense. You did a good job on this one

},
{
'NAME': 'django.contrib.auth.password_validation.CommonPasswordValidator',
'NAME': COMMON_PASSWORD_VALIDATOR,
Copy link
Contributor

Choose a reason for hiding this comment

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

@oma0256, good work here. Thanks. I see you have made some changes in the settings file. Do these two lines mean the same thing or some env variables need to be set in the env file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pbkabali Yea they do mean the same thing I was just trying to avoid pep8 issues

Copy link
Contributor

Choose a reason for hiding this comment

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

OK cool. Thanks

@oma0256 oma0256 force-pushed the ft-filter-search-articles-163383192 branch from f0bfc5e to 8fded8e Compare February 14, 2019 08:24
@karuhanga karuhanga temporarily deployed to ah-backend-summer-stagin-pr-37 February 14, 2019 08:25 Inactive
Copy link
Contributor

@fahdjamy fahdjamy left a comment

Choose a reason for hiding this comment

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

Thanks for implementing the requested changes.
LGTM right now.

@malep2007
Copy link
Contributor

@oma0256 , please resolve the merge conflicts

@oma0256 oma0256 force-pushed the ft-filter-search-articles-163383192 branch from 8fded8e to 698b4e5 Compare February 14, 2019 11:38
@karuhanga karuhanga temporarily deployed to ah-backend-summer-stagin-pr-37 February 14, 2019 11:38 Inactive
…onality of articles

 - ensure users can filter articles based on author's username, article title, tags
 - ensure users can search fo articles based on keywords

[Starts #163383192]
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

6 participants