-
Notifications
You must be signed in to change notification settings - Fork 1
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
Backend/feature/user endpoint #30
Conversation
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.
As in comments above, implement the users/{user_id}
endpoint. This shouldn't be too much work as you already have the functionality.
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.
To stay consistent with other endpoints, don't capitalize your response keys. In your case Message
-> message
. Also, next to only a message or only data. In each case provide at least the following 3 fields: message
, url
and data
. Except for delete where there is no data
# Conflicts: # backend/project/__init__.py # backend/project/__main__.py # backend/project/endpoints/index/OpenAPI_Object.json # backend/project/models/users.py # backend/tests/endpoints/conftest.py # backend/tests/models/conftest.py # backend/tests/models/course_test.py
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 good in general
Maybe add 200 to the returns for readability
and the returns in try
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.
You also forgot url
field for every internal error 500
. Don't forget to adjust the OpenAPI object with those changes also
backend/project/endpoints/users.py
Outdated
if user is None: | ||
return {"message": "User not found!"}, 404 | ||
|
||
user_js = { |
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.
You don't need this extra step, you can pass user as value of "data" field
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.
The tests fail if i don't do this
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.
That's because you register your endpoints to the API. You should still register them to the blueprint. Calling Api(bp) already sets the appropriate middleware etc.
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.
the result of user = db.session.get(userModel, user_id)
is a User(...)
object, how is this supposed to go into the json?
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.
See current version of development
this is being done all the time. This is the reason you had to make your model a dataclass.
backend/project/endpoints/users.py
Outdated
|
||
# Save the changes to the database | ||
db.session.commit() | ||
user_js = { |
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.
Same here
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.
Some small issues left
@warreprovoost resolve branch conflicts before asking for a review |
# Conflicts: # backend/project/__init__.py
@AronBuzogany my bad did not see it, should be good now |
User endpoint created
The signature of
URL.create(...)
should be the same for everyone.Pycharm added the minimal version in the requirements, is this okay?