Skip to content
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

Change Feedback Type #4728

Merged
merged 2 commits into from May 28, 2018

Conversation

@schedutron
Copy link
Member

commented May 14, 2018

Fixes #4708

Checklist

Short description of what this resolves:

The feedback score should be a float, not a string. This PR changes the feedback type to float and updates the API docs accordingly.

@schedutron

This comment has been minimized.

Copy link
Member Author

commented May 14, 2018

I think I also need to write/edit associated tests for this.

@codecov

This comment has been minimized.

Copy link

commented May 15, 2018

Codecov Report

❗️ No coverage uploaded for pull request base (development@e840409). Click here to learn what that means.
The diff coverage is 60%.

Impacted file tree graph

@@              Coverage Diff               @@
##             development    #4728   +/-   ##
==============================================
  Coverage               ?   65.31%           
==============================================
  Files                  ?      213           
  Lines                  ?     9820           
  Branches               ?        0           
==============================================
  Hits                   ?     6414           
  Misses                 ?     3406           
  Partials               ?        0
Impacted Files Coverage Δ
app/api/schema/feedbacks.py 100% <100%> (ø)
app/models/feedback.py 60% <33.33%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e840409...0d7d7b4. Read the comment docs.

@schedutron

This comment has been minimized.

Copy link
Member Author

commented May 15, 2018

Travis build is successful, but Codecov shows red flag. What should I do?

@poush

This comment has been minimized.

Copy link
Member

commented May 15, 2018

I don't think we need something like giving "4.2" rating. Also, this could also lead to giving the rating of numbers like "4.112" which makes no sense in terms of rating. I think it should be better integer.

@simarsingh24

This comment has been minimized.

Copy link
Member

commented May 16, 2018

but the user can give a 3.5 rating to an event?

@schedutron

This comment has been minimized.

Copy link
Member Author

commented May 16, 2018

@poush @simarsingh24 We can round it to the nearest 0.5 though, before storing, as (rating * 2) -> round off -> divide the result by 2. With this formula, 4.2 becomes 4.0 and 4.3 becomes 4.5.

@schedutron

This comment has been minimized.

Copy link
Member Author

commented May 16, 2018

Should I implement that?

@bhaveshAn

This comment has been minimized.

Copy link
Member

commented May 16, 2018

I think the rating which a user gives should be an Integer. And we should have a average_rating attribute (additional) of type Float in the event model to describe the average rating of an event.

@schedutron

This comment has been minimized.

Copy link
Member Author

commented May 16, 2018

@bhaveshAn Average rating sounds great! @poush @simarsingh24 Should I implement that?

@poush

This comment has been minimized.

Copy link
Member

commented May 17, 2018

I will suggest to create new issue with concept of average rating. There are various ways to do it. for example creating the separate end point to get average, calculating the average every time event detail api is called, or updating average column in event every time feedback is updated.
Let's not block this PR. Just change it to integer, create a new issue to finalise that.

@schedutron

This comment has been minimized.

Copy link
Member Author

commented May 17, 2018

@poush Okay, will create a separate issue and PR for the average rating.

@schedutron schedutron force-pushed the schedutron:feedback branch from 287bff4 to daa9cf1 May 17, 2018

@@ -5,15 +5,15 @@ class Feedback(db.Model):
"""Feedback model class"""
__tablename__ = 'feedback'
id = db.Column(db.Integer, primary_key=True)
rating = db.Column(db.String, nullable=False)
rating = db.Column(db.Integer, nullable=False)

This comment has been minimized.

Copy link
@poush

poush May 18, 2018

Member

I think this should have migration file

@iamareebjamal

This comment has been minimized.

Copy link
Member

commented May 18, 2018

Generally, all mobile UI widgets (iOS and Android) support 0.5 step for rating. We can round it easily to using 0.5 step

This will be a server limitation if only integers are allowed. Also, there should be a min-max limit of 0-5 on the field in marshmallow schema (preferably in db as well)

@schedutron

This comment has been minimized.

Copy link
Member Author

commented May 18, 2018

@iamareebjamal So should I revert to float and use the rounding-off formula mentioned before?

Or should that formula be used at the front-end, rounding off the rating before posting the feedback request, since mobile platforms already support that?

@iamareebjamal

This comment has been minimized.

Copy link
Member

commented May 18, 2018

This should also be added in the server. But let other maintainers weigh in

@poush

This comment has been minimized.

Copy link
Member

commented May 19, 2018

Despite of the support of 0.5 increment, I haven't seen much the use of it especially on mobile devices. Giving half star rating through touch panel is something not much intuitive. But yeah agree on the point that the use of half or complete star is based on the UI decision of frontend team and it should not be the limitation of API Server.

@schedutron

This comment has been minimized.

Copy link
Member Author

commented May 19, 2018

@iamareebjamal @poush @simarsingh24 So the rating field should remain of type Float?

@iamareebjamal

This comment has been minimized.

Copy link
Member

commented May 20, 2018

@schedutron schedutron force-pushed the schedutron:feedback branch 2 times, most recently from bc6adc4 to 40bdce7 May 20, 2018

@iamareebjamal

This comment has been minimized.

Copy link
Member

commented May 21, 2018

  • Round to 0.5 step
  • Implement min max constraints
self.rating = rating
rating = float(rating)
if not 0 < rating < 5:
raise ValueError, "Rating should be in range [0, 5]."

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot May 21, 2018

deprecated form of raising exception
SyntaxError: invalid syntax

This comment has been minimized.

Copy link
@iamareebjamal

iamareebjamal May 21, 2018

Member

Use marshmallow fields and validators instead

@schedutron

This comment has been minimized.

Copy link
Member Author

commented May 21, 2018

@iamareebjamal Should I implement the limits in the DB as well?

@fossasia fossasia deleted a comment from codacy-bot May 21, 2018

@bhaveshAn
Copy link
Member

left a comment

LGTM if Travis passing.

@schedutron

This comment has been minimized.

Copy link
Member Author

commented May 26, 2018

Should I delete the procedure after the update completes?

@mayank8318
Copy link
Contributor

left a comment

LGTM

@fossasia fossasia deleted a comment from codacy-bot May 27, 2018

@fossasia fossasia deleted a comment from codacy-bot May 27, 2018

@schedutron schedutron dismissed stale reviews from mayank8318, poush, and bhaveshAn via c20a122 May 27, 2018

@fossasia fossasia deleted a comment from codacy-bot May 27, 2018

@fossasia fossasia deleted a comment from codacy-bot May 27, 2018


def upgrade():
op.create_or_replace_sp(update_rating_func)
op.execute(UPDATE feedback SET rating=update_rating(rating)")

This comment has been minimized.

Copy link
@iamareebjamal

iamareebjamal May 27, 2018

Member

As reported, invalid syntax

@iamareebjamal

This comment has been minimized.

Copy link
Member

commented May 27, 2018

@schedutron Still the conflict has been resolved in wrong manner. I'd recommend that you squash your commits

@fossasia fossasia deleted a comment from codacy-bot May 27, 2018

@fossasia fossasia deleted a comment from codacy-bot May 27, 2018

@schedutron schedutron force-pushed the schedutron:feedback branch 2 times, most recently from 098dacb to 2525021 May 27, 2018

@schedutron

This comment has been minimized.

Copy link
Member Author

commented May 27, 2018

I have rebased it on development branch's head.

@iamareebjamal

This comment has been minimized.

Copy link
Member

commented May 28, 2018

@schedutron Still squash your commits as it is best practice

Change rating type to float
Remove unused import

USING rating::double precision

Add rating rounding and limits

Use modern raise syntax

Add marshmallow validation

Resolve merge conflicts

Fix typo

Add dict in error response

Use validate.Range class

Cast existing data

Use PL/PGSQL for rating update

Remove function after update

@schedutron schedutron force-pushed the schedutron:feedback branch from 2525021 to cfba14a May 28, 2018

@schedutron

This comment has been minimized.

Copy link
Member Author

commented May 28, 2018

@bhaveshAn bhaveshAn merged commit a84e05e into fossasia:development May 28, 2018

3 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Hound No violations found. Woof!
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.