Simplified tag system - #10
Conversation
There was a problem hiding this comment.
Looks very nice to me!
Maybe one thing that is not clear is the variable needed to activate it. It seems that we need to set both PYDATALAB_ENABLE_TAGS and VUE_APP_ENABLE_TAGS, am I missing something ?
I think it might be good to include the "goal" of the tags somewhere, i.e. the ability to filter by tags. Even though it could be in another PR, I think that it is precisely that possibility that makes tags useful.
What do you think ?
Only one config variable to enable tags (server side)
|
Hello! Did you do changes in Frontend? |
No visualization and front-end logic changes except the simplifications related to the fact that only global tags exist and only an admin can create them. @davidwaroquiers added the tags filtering in #12 and we will probably merge that here as well. |
DianaAliabieva
left a comment
DianaAliabieva
left a comment
There was a problem hiding this comment.
Hello! For me everything looks good, I didn't find anything anymore.
only one thing with error code
| if not updated_data: | ||
| return ( | ||
| jsonify(status="error", message="No data provided to update the tag with."), | ||
| 204, # 204: No content |
There was a problem hiding this comment.
| 204, # 204: No content | |
| 400, # 400: Bad Request |
There was a problem hiding this comment.
The HTTP specification states that a 204 No Content response must not have a body.
If you call jsonify() and return a JSON payload with a message, you are returning content. If you try to return JSON with a 204 status:
Flask/WSGI servers might strip the body automatically, meaning your "No data provided..." message gets deleted entirely.
If the request is invalid because the payload was empty, return a 400 Bad Request so the client understands it was an error and can read your message.
There was a problem hiding this comment.
Option 2:
If you truly want to return 204 No Content
If you decide that an empty payload is a success state that just requires doing nothing, you must omit the JSON body entirely.
if not updated_data:
# Flask allows returning an empty string/response for 204
return "", 204
but i prefer 400
Tags filtering
3 participants
Simplified version of tag system. Only allowing admin to create tags and no scope. Also adding variables to enable the feature.
Do you want to have another review @davidwaroquiers @DianaAliabieva ?