CB-244: Rating support in the API #154

breakbuidl · 2017-08-24T11:23:57Z

Dependent on #152

breakbuidl · 2017-08-24T11:32:28Z

critiquebrainz/ws/review/views.py

@@ -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


Need some help with this. I tried some solutions but they were not promising.

Which parameter? I don't quite understand the issue.

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.

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.

But, what if the user wants to remove the text part from text+rating review.

They can pass None (null).

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.

Is there a way to check if request JSON has a field or not?

breakbuidl · 2017-08-24T11:45:27Z

@gentlecat Had to do it this way. For some reason, requesting a review option was not showing up for me.

gentlecat · 2017-08-25T18:27:12Z

OK, I guess that's admin-only feature. Feel free to just cc me in a description like you've done here.

gentlecat · 2017-08-25T18:40:56Z

critiquebrainz/ws/review/views.py

+        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')


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.

This seems to be a part of the issue mentioned above.

gentlecat · 2017-08-25T18:42:50Z

critiquebrainz/ws/review/views.py

        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')


"Review must have either text or rating"

gentlecat · 2017-08-25T18:44:21Z

critiquebrainz/ws/review/views.py

@@ -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:


review['rating'] is redundant here, no?

Yes. Its removed.

gentlecat · 2017-08-25T18:45:18Z

critiquebrainz/ws/review/views.py

@@ -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.')


"Voting on reviews without text is not allowed"

Also mention that in the documentation.

breakbuidl · 2017-08-31T07:12:03Z

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 None.

- Skip updates for missing fields

breakbuidl · 2017-09-03T19:25:29Z

Are the changes in the commit c467a77 final? Should I update the docs accordingly?

gentlecat · 2017-09-04T00:24:28Z

critiquebrainz/ws/parser.py

-        return _k
+        if key not in _dict and not optional:
+            raise MissingDataError(key)
+        else:


else here looks unnecessary.

gentlecat · 2017-09-04T00:31:20Z

critiquebrainz/ws/review/views.py

+REVIEW_TEXT_MAX_LENGTH = 100000
+REVIEW_TEXT_MIN_LENGTH = 25
+REVIEW_RATING_MAX = 5
+REVIEW_RATING_MIN = 1


These constants should be defined in the database-related package.

gentlecat · 2017-09-04T00:36:43Z

critiquebrainz/ws/review/views.py

+        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


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.

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.

gentlecat · 2017-09-04T00:37:21Z

critiquebrainz/ws/review/views.py

+    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')


I think we discussed this, but this shouldn't be a requirement. Users don't need to think about that.

Ok. If none of the parameters are modified, it won't throw an exception and won't make a new revision as well.

gentlecat · 2017-09-04T00:37:35Z

critiquebrainz/ws/review/views.py


    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':


Yeah, this is not good.

gentlecat · 2017-09-04T00:38:15Z

critiquebrainz/ws/review/views.py

+    text, rating = fetch_params()
+    if text == 'same':
+        text = review['text']
+    if rating == 0:


Same thing with rating, really. What happens if we decide to change the range to support 0?

gentlecat · 2017-09-04T00:40:17Z

Should I update the docs accordingly?

Documentation updates for endpoints that are changing should be here as well.

breakbuidl commented Aug 24, 2017

View reviewed changes

gentlecat suggested changes Aug 29, 2017

View reviewed changes

breakbuidl force-pushed the cb-244/ws branch 5 times, most recently from 4ffbd27 to 7f3d0fe Compare August 31, 2017 07:11

gentlecat added the blocked by another PR label Sep 3, 2017

breakbuidl mentioned this pull request Sep 3, 2017

CB-244: Rating System (Docs) #150

Closed

gentlecat changed the title ~~CB-244: Rating System (Web Service)~~ CB-244: Rating support in the API Sep 3, 2017

gentlecat removed the blocked by another PR label Sep 3, 2017

Pinank Solanki added 7 commits September 3, 2017 23:44

Add rating field in review_post_handler and its test

8cbea7b

Add rating field in review_modify_handler and its test

89c0c0e

Add voting check for only-rating type of review

17af813

Remove unnecessay conditions

bf5763b

Correct grammar of exception messages

c0564f9

Modifications in review_modify_handler and its test

c467a77

- Skip updates for missing fields

Parser code clean up

e290d5c

breakbuidl force-pushed the cb-244/ws branch from 7f3d0fe to e290d5c Compare September 3, 2017 18:16

gentlecat suggested changes Sep 4, 2017

View reviewed changes

Pinank Solanki added 5 commits September 4, 2017 14:51

Assign unmodified parameters directly to previous revisions' value

dabdd74

Modify test case for no changes in review contents and a minor fix

4a8a77c

Move several review constraint definitions to db/__init__.py

9867afb

Documentation updates

d50570f

Remove redundant else clause

8dc151a

gentlecat merged commit a380e42 into metabrainz:master Sep 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CB-244: Rating support in the API #154

CB-244: Rating support in the API #154

breakbuidl commented Aug 24, 2017

breakbuidl Aug 24, 2017 •

edited

gentlecat Aug 25, 2017

breakbuidl Aug 29, 2017

gentlecat Aug 30, 2017

breakbuidl Aug 30, 2017

gentlecat Aug 30, 2017 •

edited

breakbuidl Aug 30, 2017 •

edited

gentlecat Aug 30, 2017

breakbuidl commented Aug 24, 2017

gentlecat commented Aug 25, 2017

gentlecat Aug 25, 2017

breakbuidl Aug 29, 2017

gentlecat Aug 25, 2017

gentlecat Aug 25, 2017

breakbuidl Aug 30, 2017

gentlecat Aug 25, 2017

gentlecat Aug 25, 2017

breakbuidl commented Aug 31, 2017

breakbuidl commented Sep 3, 2017

gentlecat Sep 4, 2017

gentlecat Sep 4, 2017

gentlecat Sep 4, 2017

breakbuidl Sep 4, 2017

gentlecat Sep 4, 2017

breakbuidl Sep 4, 2017

gentlecat Sep 4, 2017

gentlecat Sep 4, 2017

gentlecat commented Sep 4, 2017

CB-244: Rating support in the API #154

CB-244: Rating support in the API #154

Conversation

breakbuidl commented Aug 24, 2017

breakbuidl Aug 24, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gentlecat Aug 30, 2017 • edited

Choose a reason for hiding this comment

breakbuidl Aug 30, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

breakbuidl commented Aug 24, 2017

gentlecat commented Aug 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

breakbuidl commented Aug 31, 2017

breakbuidl commented Sep 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gentlecat commented Sep 4, 2017

breakbuidl Aug 24, 2017 •

edited

gentlecat Aug 30, 2017 •

edited

breakbuidl Aug 30, 2017 •

edited