-
Notifications
You must be signed in to change notification settings - Fork 0
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
Draft: Implement new API routes to fetch blog posts by authorIds
and to update a post
#2
base: main
Are you sure you want to change the base?
Changes from 21 commits
9c54e06
604f094
6904073
6589caf
3774363
68326a5
470cbf2
44f5f61
d660db7
3a6f17b
9612dc6
76e5d03
2e26c4b
cb60657
04b70b7
c5b84d0
648c0f8
3a1e6b6
c208ca4
5529ef6
9a4d4c4
dbf2ea6
24f1be2
732a030
b01a21f
6773aa4
3ac2e08
dd9e9a9
c3f5764
7f3a782
c88810a
1da081a
8674425
1855fc0
58917da
c4aeed8
7107958
f630b16
9fcc11f
0f103f9
99c75f5
a9fa08b
03c8bd5
3c0a5f0
24fa5d1
1321643
7b9cb9c
d90805e
67ca0ef
f4e0b34
52bc8b4
dd7ca19
68bb986
3c3abdc
e13bc1c
1d7f5ca
d160ce1
f47e531
e15c332
dd272e8
06088b0
1d058c0
5437c1e
c712f5a
c52b43c
d0a273e
1f55a4f
65ce818
7a69b51
5dc1b3d
39f41c7
56a7666
c628c18
088d1cd
1e15e1a
327719b
26dfb89
9067923
cb0a60c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -163,4 +163,7 @@ cython_debug/ | |
*.db | ||
|
||
# OS X | ||
.DS_Store | ||
.DS_Store | ||
|
||
# Scratch work | ||
scratch_work.txt |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,12 @@ | |
from db.utils import row_to_dict | ||
from middlewares import auth_required | ||
|
||
import crud | ||
|
||
|
||
POSTS_SORT_BY_OPTIONS: list[str] = ["id", "reads", "likes", "popularity"] | ||
POSTS_SORT_DIRECTION_OPTIONS: list[str] = ["asc", "desc"] | ||
|
||
|
||
@api.post("/posts") | ||
@auth_required | ||
|
@@ -37,3 +43,90 @@ def posts(): | |
db.session.commit() | ||
|
||
return row_to_dict(post), 200 | ||
|
||
|
||
@api.route("/posts", methods=["GET"]) | ||
@auth_required | ||
def fetch_posts(): | ||
""" | ||
Fetch blog posts that have at least one of the authors specified. | ||
""" | ||
author_ids_input: str = request.args.get("authorIds", None) | ||
huaszu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
if author_ids_input is None: | ||
return jsonify({"error": "Please identify the author(s) whose posts to fetch using the query parameter key `authorIds`."}), 400 | ||
|
||
sort_by_input: str = request.args.get("sortBy", "id") | ||
huaszu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if sort_by_input not in POSTS_SORT_BY_OPTIONS: | ||
return jsonify({"error": "Unacceptable value for `sortBy` parameter. We can sort by id, reads, likes, or popularity."}), 400 | ||
|
||
direction_input: str = request.args.get("direction", "asc") | ||
huaszu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if direction_input not in POSTS_SORT_DIRECTION_OPTIONS: | ||
return jsonify({"error": "Unacceptable value for `direction` parameter. We only accept asc or desc."}), 400 | ||
|
||
author_ids: set[int] = set() | ||
huaszu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
try: | ||
for author_id_input in author_ids_input.split(","): | ||
huaszu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
author_id = int(author_id_input) | ||
if crud.check_user_exists(author_id): | ||
author_ids.add(author_id) | ||
except: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i'm not sure what in your try would hit the except - does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no, i want to hit the except when the request has an invalid There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. an update: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i suppose we could be more specific to the requester whether the error was that the query parameter value had no integers at all (e.g., "all"), or had integers but incorrectly separated (e.g., "1+2"), or so on |
||
return jsonify({"error": "Please provide a query parameter value for `authorIds` as a number or as numbers separated by commas, such as '1,5'."}), 400 | ||
|
||
if not author_ids: # Also helps to avoid the problem that subsequently | ||
# running `Post.query.with_parent(user).all()` on users that do not | ||
# exist will give an error | ||
return jsonify({"error": "None of the author id(s) you requested exist in the database."}), 200 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's a little confusing to return a response that has a 200 status and also an error message. Maybe make the error a warning? Or message? Or maybe there's a more appropriate status code for this kind of situation? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if you want to use another status code you can look through existing ones and their meaning here - https://developer.mozilla.org/en-US/docs/Web/HTTP/Status There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i looked through the status codes. clarifying that there is not actually an error seems the way to go. does this work: 3ac2e08 ? or did you mean a warning as in the Python |
||
|
||
posts_of_authors: set[Post] = set() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why is this a set over a regular array? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. at first i made this an array. i changed my mind because when i read onward in the project instructions to the Part 2 feature, i noticed that for a post, authorIds is an array of integers. i paid more attention to the responses i got to my test GET requests and anecdotally saw that there exist posts in the seeded db that have more than one author. if a post can have multiple authors, then here when we query for posts by author, a query for a different author can result in a duplicate post. i wanted the set to help avoid duplication. what benefits of an array might you be interested in that we lose here? side note: perhaps i could have made the above observation more easily if i looked over the db first. i have only used PostgreSQL before and haven't figured out how to use SQLite. i looked online and made a cursory attempt that did not work. |
||
|
||
for author_id in author_ids: | ||
for post in Post.get_posts_by_user_id(author_id): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe another chance for fancy list comprehension on the inner for loop There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i was thinking of
however, does this mean we recreate the set every time we iterate on a my logic was to have a set that can continue to grow with more posts as we go through more authors and add each author's posts to the set. or did you have something else in mind using comprehension? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oooh i came up with a different way that incorporates your feedback to use comprehension and solves the problem i considered in the previous comment. i committed here.
i initialize a set that i intend to be a superset of posts. in each iteration of the outer for loop, make a set of that author's posts. update this set into the superset. my first time using the now i still have the same number of lines of code. i don't know about the benefits of:
the differences between these benefits may vary depending on how often in the data we expect there to be duplicates filtered out and not added to the final set (e.g., if many posts are co-authored and it is likely when querying by author to get the same posts again and again)? an argument could be made that now the code is more readable because we can see that:
also, the before implementation may be more extensible if we foresee wanting to do other manipulating of post by individual post and |
||
posts_of_authors.add(post) | ||
|
||
if not posts_of_authors: # If posts_of_authors is empty, the later code to | ||
# populate the `posts_data` dictionary will give an error so let's | ||
# avoid that | ||
return jsonify({"posts": []}), 200 | ||
|
||
posts_data: dict[int, dict] = {} | ||
|
||
for post in posts_of_authors: | ||
posts_data[post.id] = {"id": post.id, # This key-value pair is redundant with the outer dictionary key. What problems are we causing? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why are you saving the id as both the key and value? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note: line numbers have changed with the latest commits so i am referring to the latest code right now. since we want to be able to sort on id, reads, likes, or popularity, i thought i would make sure all four of these are organized in a way that line 109 can use the function from line 106 to access any of these four as a key to sort on. what do you think would be better to do? @hdenisenko |
||
"likes": post.likes, | ||
"popularity": post.popularity, | ||
"reads": post.reads, | ||
"tags": post._tags.split(","), | ||
"text": post.text} | ||
# Alternative: For each post, make a dictionary in the format of the | ||
# inner dictionary above. Have a list of these dictionaries. Later | ||
# can sort the list as desired. However, generating this outer | ||
# dictionary of dictionaries seems more extensible and has better time | ||
# complexity if we want to look up a post (though arguably we could | ||
# just access the db to get a post's info - depends on the context, | ||
# what the API user might want to do in the future, what access the | ||
# user has, whom and what we are building for, et al) | ||
|
||
def sort_posts_on(item): | ||
huaszu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return item[1][sort_by_input] | ||
|
||
if direction_input == "asc": | ||
reverse_boolean: bool = False | ||
else: | ||
reverse_boolean: bool = True | ||
|
||
sorted_posts: list[tuple] = sorted(posts_data.items(), key=sort_posts_on, reverse=reverse_boolean) | ||
huaszu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
# Alternative: Have SQLAlchemy help sort posts when querying database on line 85. | ||
# Not sure how much this alternative helps because we query database by | ||
# author id and ultimately we want to sort not on author id, but on | ||
# post id, reads, likes, or popularity. There could be some benefit of, | ||
# for each author, sorting by the desired one of the four sort by options | ||
# at the point of querying the database, and then preparing the final sort | ||
# later. That could be investigated. | ||
|
||
result: list[dict] = [] | ||
for post_response in sorted_posts: | ||
result.append(post_response[1]) | ||
huaszu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
return jsonify({"posts": result}), 200 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
from db.shared import db | ||
huaszu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
from db.models.user import User | ||
|
||
def check_user_exists(user_id): | ||
"""Check by user id whether or not a user exists.""" | ||
|
||
return User.query.get(user_id) is not None |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
Instructions: Complete one project to proceed to the next steps. You'll receive tailored feedback on your programming skills and a recommendation on the best next step to get job ready. | ||
|
||
The goal of this assessment is to test your backend development skills. | ||
|
||
You will be building on top of a simple JSON API using Python (Flask). If you have never written a JSON API using those technologies before, this https://auth0.com/blog/developing-restful-apis-with-python-and-flask/ may be a good resource that may help you. | ||
|
||
The assessment involves building two new routes for a blog post API. Detailed instructions will be available once you click start. Your assessment will be graded based on this rubric. https://drive.google.com/file/d/103oOiqjxd_N1JckefKqPJjndqX1qvqdL/view | ||
|
||
FEEDBACK: | ||
WHAT YOU CAN EXPECT | ||
|
||
You will receive comprehensive review & feedback from our experienced engineers. You will also receive guidance on what to do next on the Feedback page. | ||
|
||
HOW LONG IT WILL NEED | ||
|
||
3 - 10 days |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like the function they gave you does validation this way
'''
validation
'''
Maybe you can do the same in your function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so far your suggestion appears to work. committed here.
i added the code you suggested. then i made this request and got this 401:
next i logged in:
now this request returned what i expected and 200 instead of the 401: