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
CB-244: Rating support in the API #154
Conversation
critiquebrainz/ws/review/views.py
Outdated
@@ -231,21 +233,30 @@ def review_modify_handler(review_id, user): | |||
|
|||
:resheader Content-Type: *application/json* | |||
""" | |||
|
|||
# TODO(psolanki): One caveat is that client will have to pass the unmodified parameter with previous revision's value | |||
# otherwise it will be set to None |
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.
Need some help with this. I tried some solutions but they were not promising.
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.
Which parameter? I don't quite understand the issue.
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.
Suppose the review is of text+rating type. If the user wants to change just the rating and passes just the rating parameter, text will be set to None(because its optional). In order to do so, text (unmodified parameter) must be passed with previous revision's text value.
Now this does not seem to be a very elegant solution.
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.
Since that's an interface for updating reviews I don't see anything wrong without skipping updates for missing fields. If text is not provided then it doesn't need to be modified.
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.
But, what if the user wants to remove the text part from text+rating review.
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.
They can pass None
(null
).
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.
There's a problem with this. Since the parameters are optional, not passing them is equal to passing them with None. Apparently, there's nothing to differentiate if the user wants a certain field unmodified or removed.
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.
Is there a way to check if request JSON has a field or not?
@gentlecat Had to do it this way. For some reason, requesting a review option was not showing up for me. |
OK, I guess that's admin-only feature. Feel free to just cc me in a description like you've done here. |
critiquebrainz/ws/review/views.py
Outdated
text = Parser.string('json', 'text', min=REVIEW_TEXT_MIN_LENGTH, max=REVIEW_TEXT_MAX_LENGTH, optional=True) | ||
rating = Parser.int('json', 'rating', min=REVIEW_RATING_MIN, max=REVIEW_RATING_MAX, optional=True) | ||
if text is None and rating is None: | ||
raise InvalidRequest(desc='Text part and rating part of a review cannot be None simultaneously') |
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.
I don't think this should be an invalid request case. It should be perfectly fine to submit an update without any changes. If that special case needs to be handled somehow then we should do that on the server and not ask users to worry about 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.
This seems to be a part of the issue mentioned above.
critiquebrainz/ws/review/views.py
Outdated
license_choice = Parser.string('json', 'license_choice') | ||
language = Parser.string('json', 'language', min=2, max=3, optional=True) or 'en' | ||
if text is None and rating is None: | ||
raise InvalidRequest(desc='Text part and rating part of a review cannot be None simultaneously') |
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.
"Review must have either text or rating"
critiquebrainz/ws/review/views.py
Outdated
@@ -530,6 +545,8 @@ def fetch_params(): | |||
vote = fetch_params() | |||
if str(review["user_id"]) == user.id: | |||
raise InvalidRequest(desc='You cannot rate your own review.') | |||
if review['rating'] and review["text"] is None: |
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.
review['rating']
is redundant here, no?
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.
Yes. Its removed.
critiquebrainz/ws/review/views.py
Outdated
@@ -530,6 +545,8 @@ def fetch_params(): | |||
vote = fetch_params() | |||
if str(review["user_id"]) == user.id: | |||
raise InvalidRequest(desc='You cannot rate your own review.') | |||
if review['rating'] and review["text"] is None: | |||
raise InvalidRequest(desc='You cannot vote for an only-rating type of review.') |
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.
"Voting on reviews without text is not allowed"
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.
Also mention that in the documentation.
4ffbd27
to
7f3d0fe
Compare
With code clean-up in parser, two things are added. A check is included if the request has a certain field and a mandatory field can now have its value as |
- Skip updates for missing fields
7f3d0fe
to
e290d5c
Compare
Are the changes in the commit c467a77 final? Should I update the docs accordingly? |
critiquebrainz/ws/parser.py
Outdated
return _k | ||
if key not in _dict and not optional: | ||
raise MissingDataError(key) | ||
else: |
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.
else
here looks unnecessary.
critiquebrainz/ws/review/views.py
Outdated
REVIEW_TEXT_MAX_LENGTH = 100000 | ||
REVIEW_TEXT_MIN_LENGTH = 25 | ||
REVIEW_RATING_MAX = 5 | ||
REVIEW_RATING_MIN = 1 |
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.
These constants should be defined in the database-related package.
critiquebrainz/ws/review/views.py
Outdated
try: | ||
text = Parser.string('json', 'text', min=REVIEW_TEXT_MIN_LENGTH, max=REVIEW_TEXT_MAX_LENGTH) | ||
except MissingDataError: | ||
text = 'same' # Assign temporary value which indicates no modification |
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.
This doesn't seem right. What if the review text is actually same
, then the comparisons afterwards are going to break. Sure, it doesn't match the constraints for the text, so it's currently not possible to write a review like that. But this might change in the future and in any case that seems very hacky way of doing things.
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.
Agreed. This is not subtle. I added some changes which are much cleaner than this. Don't know why I couldn't think of such a simple solution earlier.
critiquebrainz/ws/review/views.py
Outdated
if rating == 0: | ||
rating = review['rating'] | ||
if (text == review['text']) and (rating == review['rating']): | ||
raise InvalidRequest(desc='Either text or rating should be edited to update the review') |
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.
I think we discussed this, but this shouldn't be a requirement. Users don't need to think about that.
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.
Ok. If none of the parameters are modified, it won't throw an exception and won't make a new revision as well.
critiquebrainz/ws/review/views.py
Outdated
|
||
review = get_review_or_404(review_id) | ||
if review["is_hidden"]: | ||
raise NotFound("Review has been hidden.") | ||
if str(review["user_id"]) != user.id: | ||
raise AccessDenied | ||
text = fetch_params() | ||
text, rating = fetch_params() | ||
if text == 'same': |
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.
Yeah, this is not good.
critiquebrainz/ws/review/views.py
Outdated
text, rating = fetch_params() | ||
if text == 'same': | ||
text = review['text'] | ||
if rating == 0: |
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 thing with rating, really. What happens if we decide to change the range to support 0
?
Documentation updates for endpoints that are changing should be here as well. |
Dependent on #152