diff --git a/db/manifest b/db/manifest index 00d3c8c44..6de78a088 100755 --- a/db/manifest +++ b/db/manifest @@ -297,6 +297,7 @@ alter_genre_player_unique_constraint.sql musician_search.sql enhance_band_profile.sql alter_band_profile_rate_defaults.sql +<<<<<<< HEAD repair_band_profile.sql jam_track_onboarding_enhancements.sql jam_track_name_drop_unique.sql @@ -319,4 +320,5 @@ session_controller.sql jam_tracks_bpm.sql profile_teacher.sql populate_languages.sql -populate_subjects.sql \ No newline at end of file +populate_subjects.sql +reviews.sql \ No newline at end of file diff --git a/db/up/reviews.sql b/db/up/reviews.sql new file mode 100644 index 000000000..89a30ee32 --- /dev/null +++ b/db/up/reviews.sql @@ -0,0 +1,23 @@ +CREATE TABLE reviews ( + id VARCHAR(64) PRIMARY KEY DEFAULT uuid_generate_v4() NOT NULL, + user_id VARCHAR(64) NOT NULL REFERENCES users(id) ON DELETE CASCADE, + target_id VARCHAR(64) NOT NULL, + target_type VARCHAR(32) NOT NULL, + description VARCHAR, + rating INT NOT NULL, + deleted_by_user_id VARCHAR(64) REFERENCES users(id) ON DELETE SET NULL, + deleted_at TIMESTAMP WITHOUT TIME ZONE DEFAULT NULL, + created_at TIMESTAMP WITHOUT TIME ZONE DEFAULT NOW() NOT NULL, + updated_at TIMESTAMP WITHOUT TIME ZONE DEFAULT NOW() NOT NULL +); + +CREATE TABLE review_summaries ( + id VARCHAR(64) PRIMARY KEY DEFAULT uuid_generate_v4() NOT NULL, + target_id VARCHAR(64) NOT NULL, + target_type VARCHAR(32) NOT NULL, + avg_rating FLOAT NOT NULL, + wilson_score FLOAT NOT NULL, + review_count INT NOT NULL, + created_at TIMESTAMP WITHOUT TIME ZONE DEFAULT NOW() NOT NULL, + updated_at TIMESTAMP WITHOUT TIME ZONE DEFAULT NOW() NOT NULL +); \ No newline at end of file diff --git a/ruby/lib/jam_ruby.rb b/ruby/lib/jam_ruby.rb index 2502ca2a7..8ab6f1269 100755 --- a/ruby/lib/jam_ruby.rb +++ b/ruby/lib/jam_ruby.rb @@ -116,6 +116,8 @@ require "jam_ruby/models/ip_blacklist" require "jam_ruby/models/user_blacklist" require "jam_ruby/models/fraud_alert" require "jam_ruby/models/fingerprint_whitelist" +require "jam_ruby/models/review" +require "jam_ruby/models/review_summary" require "jam_ruby/models/rsvp_request" require "jam_ruby/models/rsvp_slot" require "jam_ruby/models/rsvp_request_rsvp_slot" diff --git a/ruby/lib/jam_ruby/models/review.rb b/ruby/lib/jam_ruby/models/review.rb new file mode 100644 index 000000000..157790e43 --- /dev/null +++ b/ruby/lib/jam_ruby/models/review.rb @@ -0,0 +1,93 @@ +module JamRuby + class Review < ActiveRecord::Base + include HtmlSanitize + html_sanitize strict: [:description] + + attr_accessible :target, :rating, :description, :user, :user_id, :target_id, :target_type + belongs_to :target, polymorphic: true + belongs_to :user, foreign_key: 'user_id', class_name: "JamRuby::User" + belongs_to :deleted_by_user, foreign_key: 'deleted_by_user_id', class_name: "JamRuby::User" + + scope :available, -> { where("deleted_at iS NULL") } + scope :all, -> { select("*") } + + validates :description, length: {maximum: 16000}, no_profanity: true, :allow_blank => true + validates :rating, presence: true, numericality: {only_integer: true, minimum: 1, maximum: 5} + + validates :target, presence: true + validates :user_id, presence: true + validates :target_id, uniqueness: {scope: :user_id, message: "There is already a review for this User and Target."} + + after_save :reduce + + + def self.index(options={}) + if options.key?(:include_deleted) + arel = Review.all + else + arel = Review.available + end + + if options.key?(:target_id) + arel = arel.where("target_id=?", options[:target_id]) + end + + if options.key?(:user_id) + arel = arel.where("user_id=?", options[:user_id]) + end + + arel + end + + # Create review_summary records by grouping reviews + def self.reduce_all + ReviewSummary.transaction do + ReviewSummary.destroy_all + Review.select("target_id, target_type AS target_type, AVG(rating) as avg_rating, count(*) as review_count, SUM(CASE WHEN rating>=3.0 THEN 1 ELSE 0 END) AS pos_count") + .where("deleted_at IS NULL") + .group("target_type, target_id") + .each do |r| + wilson_score = ci_lower_bound(r.pos_count, r.review_count) + ReviewSummary.create!( + target_id: r.target_id, + target_type: r.target_type, + avg_rating: r.avg_rating, + wilson_score: wilson_score, + review_count: r.review_count + ) + end + end + end + + # http://www.evanmiller.org/how-not-to-sort-by-average-rating.html + def self.ci_lower_bound(pos, n, confidence=0.95) + pos=pos.to_f + n=n.to_f + return 0 if n == 0 + z = 1.96 # Statistics2.pnormaldist(1-(1-confidence)/2) + phat = 1.0*pos/n + (phat + z*z/(2*n) - z * Math.sqrt((phat*(1-phat)+z*z/(4*n))/n))/(1+z*z/n) + end + + def reduce + ReviewSummary.transaction do + ReviewSummary.where(target_type: target_type, target_id: target_id).destroy_all + + Review.select("target_id, target_type AS target_type, AVG(rating) as avg_rating, count(*) as review_count, SUM(CASE WHEN rating>=3.0 THEN 1 ELSE 0 END) AS pos_count") + .where("deleted_at IS NULL") + .where(target_type: target_type, target_id: target_id) + .group("target_type, target_id") + .each do |r| + wilson_score = Review.ci_lower_bound(r.pos_count, r.review_count) + ReviewSummary.create!( + target_id: r.target_id, + target_type: r.target_type, + avg_rating: r.avg_rating, + wilson_score: wilson_score, + review_count: r.review_count + ) + end + end + end + end +end \ No newline at end of file diff --git a/ruby/lib/jam_ruby/models/review_summary.rb b/ruby/lib/jam_ruby/models/review_summary.rb new file mode 100644 index 000000000..fb3833f5d --- /dev/null +++ b/ruby/lib/jam_ruby/models/review_summary.rb @@ -0,0 +1,43 @@ +module JamRuby + class ReviewSummary < ActiveRecord::Base + attr_accessible :target, :target_id, :target_type, :avg_rating, :wilson_score, :review_count + belongs_to :target, polymorphic: true + + validates :avg_rating, presence:true, numericality: true + validates :review_count, presence:true, numericality: {only_integer: true} + validates :wilson_score, presence:true, numericality: {greater_than:0, less_than:1} + validates :target_id, presence:true, uniqueness:true + + class << self + + # Query review_summaries using target type, id, and minimum review count + # * target_type: Only return review summaries for given target type + # * target_id: Only return review summary for given target type + # * minimum_reviews: Only return review summary made up of at least this many reviews + # * arel: start with pre-queried reviews (arel object) + # sorts by wilson score + def index(options={}) + options ||= {} + if (options.key?(:arel)) + arel = options[:arel].order("wilson_score DESC") + else + arel = ReviewSummary.order("wilson_score DESC") + end + + if (options.key?(:target_type)) + arel = arel.where("target_type=?", options[:target_type]) + end + + if (options.key?(:target_id)) + arel = arel.where("target_id=?", options[:target_id]) + end + + if (options.key?(:minimum_reviews)) + arel = arel.where("review_count>=?", options[:minimum_reviews]) + end + + arel + end + end + end +end \ No newline at end of file diff --git a/ruby/lib/jam_ruby/models/user.rb b/ruby/lib/jam_ruby/models/user.rb index 4767b6da9..3703e2e53 100644 --- a/ruby/lib/jam_ruby/models/user.rb +++ b/ruby/lib/jam_ruby/models/user.rb @@ -49,6 +49,9 @@ module JamRuby # authorizations (for facebook, etc -- omniauth) has_many :user_authorizations, :class_name => "JamRuby::UserAuthorization" + has_many :reviews, :class_name => "JamRuby::Review" + has_many :review_summaries, :class_name => "JamRuby::ReviewSummary" + # calendars (for scheduling NOT in music_session) has_many :calendars, :class_name => "JamRuby::Calendar" diff --git a/ruby/spec/factories.rb b/ruby/spec/factories.rb index f7763f6dd..27435b204 100644 --- a/ruby/spec/factories.rb +++ b/ruby/spec/factories.rb @@ -140,7 +140,7 @@ FactoryGirl.define do end end end - + factory :music_session, :class => JamRuby::MusicSession do sequence(:name) { |n| "Music Session #{n}" } sequence(:description) { |n| "Music Session Description #{n}" } diff --git a/ruby/spec/jam_ruby/models/review_spec.rb b/ruby/spec/jam_ruby/models/review_spec.rb new file mode 100644 index 000000000..7a5851948 --- /dev/null +++ b/ruby/spec/jam_ruby/models/review_spec.rb @@ -0,0 +1,169 @@ +require 'spec_helper' + +describe Review do + + shared_examples_for :review do |target, target_type| + before(:each) do + Review.delete_all + User.delete_all + @user = FactoryGirl.create(:user) + end + + after(:all) do + Review.delete_all + User.delete_all + end + + context "validates review" do + it "blank target" do + review = Review.create() + review.valid?.should be_false + review.errors[:target].should == ["can't be blank"] + end + + it "no rating" do + review = Review.create(target:target) + review.valid?.should be_false + review.errors[:rating].should include("can't be blank") + review.errors[:rating].should include("is not a number") + end + + it "no user" do + review = Review.create(target:target, rating:3) + review.valid?.should be_false + review.errors[:user_id].should include("can't be blank") + end + + it "complete" do + review = Review.create(target:target, rating:3, user:@user) + review.valid?.should be_true + end + + it "unique" do + review = Review.create(target:target, rating:3, user:@user) + review.valid?.should be_true + + review2 = Review.create(target:target, rating:3, user:@user) + review2.valid?.should be_false + end + + it "reduces" do + review = Review.create(target:target, rating:3, user:@user) + review.valid?.should be_true + + review2 = Review.create(target:target, rating:5, user:FactoryGirl.create(:user)) + review2.valid?.should be_true + Review.should have(2).items + Review.index.should have(2).items + + # Reduce and check: + ReviewSummary.should have(1).items + ReviewSummary.first.avg_rating.should eq(4.0) + + ws_orig = ReviewSummary.first.wilson_score + avg_orig = ReviewSummary.first.avg_rating + + # Create some more and verify: + 5.times {Review.create(target:target, rating:5, user:FactoryGirl.create(:user))} + Review.index.should have(7).items + ReviewSummary.should have(1).items + + ReviewSummary.first.wilson_score.should > ws_orig + ReviewSummary.first.avg_rating.should > avg_orig + + end + end # context + + context "validates review summary" do + it "blank target" do + review_summary = ReviewSummary.create() + review_summary.valid?.should be_false + review_summary.errors[:target_id].should == ["can't be blank"] + end + + it "no rating" do + review_summary = ReviewSummary.create(target:target) + review_summary.valid?.should be_false + review_summary.errors[:target].should be_empty + review_summary.errors[:avg_rating].should include("can't be blank") + review_summary.errors[:avg_rating].should include("is not a number") + end + + it "no score" do + review_summary = ReviewSummary.create(target:target, avg_rating:3.2) + review_summary.valid?.should be_false + review_summary.errors[:target].should be_empty + review_summary.errors[:avg_rating].should be_empty + review_summary.errors[:wilson_score].should include("can't be blank") + review_summary.errors[:wilson_score].should include("is not a number") + end + + it "no count" do + review_summary = ReviewSummary.create(target:target, avg_rating:3.2, wilson_score:0.95) + review_summary.valid?.should be_false + review_summary.errors[:review_count].should include("can't be blank") + end + + it "complete" do + review_summary = ReviewSummary.create(target:target, avg_rating:3.2, wilson_score:0.95, review_count: 15) + review_summary.valid?.should be_true + end + + it "unique" do + review = ReviewSummary.create(target:target, avg_rating:3, wilson_score:0.82, review_count:14) + review.valid?.should be_true + + review2 = ReviewSummary.create(target:target, avg_rating:3.22, wilson_score:0.91, review_count:12) + review2.valid?.should be_false + end + + it "reduces and queries" do + review = Review.create(target:target, rating:3, user:@user) + review.valid?.should be_true + review2 = Review.create(target:target, rating:5, user:FactoryGirl.create(:user)) + review2.valid?.should be_true + Review.should have(2).items + + ReviewSummary.should have(1).items + ReviewSummary.first.avg_rating.should eq(4.0) + + ws_orig = ReviewSummary.first.wilson_score + avg_orig = ReviewSummary.first.avg_rating + + + # Create some more and verify: + 5.times {Review.create(target:target, rating:5, user:FactoryGirl.create(:user))} + ReviewSummary.should have(1).items + ReviewSummary.first.wilson_score.should > ws_orig + ReviewSummary.first.avg_rating.should > avg_orig + + + # Create some more with a different target and verify: + target2=FactoryGirl.create(:jam_track) + 5.times {Review.create(target:target2, rating:5, user:FactoryGirl.create(:user))} + Review.index.should have(12).items + Review.index(target_id: target2).should have(5).items + summaries = ReviewSummary.index() + summaries.should have(2).items + summaries[0].wilson_score.should > summaries[1].wilson_score + + summaries = ReviewSummary.index(target_id: target2) + summaries.should have(1).items + summaries[0].target_id.should eq(target2.id) + + summaries = ReviewSummary.index(target_type: "JamRuby::JamTrack") + summaries.should have(2).items + + summaries = ReviewSummary.index(minimum_reviews: 6) + summaries.should have(1).items + end + end + end + + describe "with a jamtrack" do + @jam_track = FactoryGirl.create(:jam_track) + it_behaves_like :review, @jam_track, "jam_track" + end + + +end \ No newline at end of file diff --git a/web/README.md b/web/README.md index 5fa0629f8..730c0f2f5 100644 --- a/web/README.md +++ b/web/README.md @@ -2,5 +2,4 @@ Jasmine Javascript Unit Tests ============================= -Open browser to localhost:3000/teaspoon - +Open browser to localhost:3000/teaspoon \ No newline at end of file diff --git a/web/app/controllers/api_reviews_controller.rb b/web/app/controllers/api_reviews_controller.rb new file mode 100644 index 000000000..a7ace92af --- /dev/null +++ b/web/app/controllers/api_reviews_controller.rb @@ -0,0 +1,66 @@ +require 'sanitize' +class ApiReviewsController < ApiController + before_filter :api_signed_in_user, :except => [:index] + before_filter :lookup_review_summary, :only => [:details] + before_filter :lookup_review, :only => [:update, :delete, :show] + + respond_to :json + + # List review summaries according to params: + def index + summaries = ReviewSummary.index(params[:review]) + @reviews = summaries.paginate(page: params[:page], per_page: params[:per_page]) + respond_with @reviews, responder: ApiResponder, :status => 200 + end + + # Create a review: + def create + @review = Review.new + @review.target_id = params[:target_id] + @review.user = current_user + @review.rating = params[:rating] + @review.description = params[:description] + @review.target_type = params[:target_type] + @review.save + respond_with_model(@review) + end + + # List reviews matching targets for given review summary: + def details + reviews = Review.index(:target_id=>@review_summary.target_id) + @reviews = reviews.paginate(page: params[:page], per_page: params[:per_page]) + respond_with @reviews, responder: ApiResponder, :status => 200 + end + + # Update a review: + def update + mods = params[:mods] + if mods.present? + @review.rating = mods[:rating] if mods.key?(:rating) + @review.description = mods[:description] if mods.key?(:description) + @review.save + end + respond_with_model(@review) + end + + # Mark a review as deleted: + def delete + @review.deleted_at = Time.now() + @review + @review.save + render :json => {}, status: 204 + end + +private + + def lookup_review_summary + @review_summary = ReviewSummary.find(params[:review_summary_id]) + end + + def lookup_review + arel = Review.where("id=?", params[:id]) + arel = arel.where("user_id=?", current_user) unless current_user.admin + @review = arel.first + raise ActiveRecord::RecordNotFound, "Couldn't find review matching #{arel}" if @review.nil? + end +end diff --git a/web/config/routes.rb b/web/config/routes.rb index c3a0e0118..22268ee16 100644 --- a/web/config/routes.rb +++ b/web/config/routes.rb @@ -326,6 +326,12 @@ SampleApp::Application.routes.draw do match '/users/authorizations/google' => 'api_users#google_auth', :via => :get match '/users/:id/set_password' => 'api_users#set_password', :via => :post + match '/reviews' => 'api_reviews#index', :via => :get + match '/reviews' => 'api_reviews#create', :via => :post + match '/reviews/:id' => 'api_reviews#update', :via => :post + match '/reviews/:id' => 'api_reviews#delete', :via => :delete + match '/reviews/details/:review_summary_id' => 'api_users#details', :via => :get, :as => 'api_summary_reviews' + # recurly match '/recurly/create_account' => 'api_recurly#create_account', :via => :post match '/recurly/delete_account' => 'api_recurly#delete_account', :via => :delete diff --git a/web/spec/controllers/api_reviews_controller_spec.rb b/web/spec/controllers/api_reviews_controller_spec.rb new file mode 100644 index 000000000..29a670c28 --- /dev/null +++ b/web/spec/controllers/api_reviews_controller_spec.rb @@ -0,0 +1,120 @@ +require 'spec_helper' + +describe ApiReviewsController do + render_views + before(:all) do + @logged_in_user = FactoryGirl.create(:user) + end + + before(:each) do + Review.destroy_all + ReviewSummary.destroy_all + @user= FactoryGirl.create(:user) + @target= FactoryGirl.create(:jam_track) + controller.current_user = @logged_in_user + end + + after(:all) do + Review.destroy_all + ReviewSummary.destroy_all + User.destroy_all + JamTrack.destroy_all + end + + describe "create" do + it "successful" do + post :create, rating:3, description:"it was ok", target_id: @target.id, target_type:"JamRuby::JamTrack", format: 'json' + response.should be_success + Review.index.should have(1).items + end + end + + describe "update" do + before :each do + @review=Review.create!(target:@target, rating:5, description: "blah", user_id: @logged_in_user.id) + end + + it "basic" do + post :update, id:@review.id, mods: {rating:4, description: "not blah"}, :format=>'json' + response.should be_success + @review.reload + @review.rating.should eq(4) + @review.description.should eq("not blah") + end + + it "bad identifier" do + post :update, id:2112, mods: {rating:4, description: "not blah"}, :format=>'json' + response.status.should eql(404) + end + end + + describe "delete" do + before :each do + @review=Review.create!(target:@target, rating:5, description: "blah", user_id: @logged_in_user.id) + end + + it "marks review as deleted" do + delete :delete, id:@review.id + response.should be_success + Review.index.should have(0).items + Review.index(include_deleted:true).should have(1).items + end + end + + describe "indexes" do + before :each do + @target2=FactoryGirl.create(:jam_track) + + 7.times { Review.create!(target:@target, rating:4, description: "blah", user_id: FactoryGirl.create(:user).id) } + 5.times { Review.create!(target:@target2, rating:4, description: "blah", user_id: FactoryGirl.create(:user).id) } + end + + it "review summaries" do + get :index, format: 'json' + response.should be_success + json = JSON.parse(response.body) + json.should have(0).items + + ReviewSummary.index.should have(0).items + Review.reduce() + ReviewSummary.index.should have(2).items + get :index, format: 'json' + json = JSON.parse(response.body) + json.should have(2).item + end + + it "details" do + ReviewSummary.index.should have(0).items + Review.reduce() + ReviewSummary.index.should have(2).items + + summaries = ReviewSummary.index + get :details, :review_summary_id=>summaries[0].id, format: 'json' + response.should be_success + json = JSON.parse(response.body) + json.should have(7).items + + get :details, :review_summary_id=>summaries[1].id, format: 'json' + response.should be_success + json = JSON.parse(response.body) + json.should have(5).items + end + + it "paginates details" do + ReviewSummary.index.should have(0).items + Review.reduce() + summaries = ReviewSummary.index + summaries.should have(2).items + + get :details, review_summary_id:summaries[0].id, page: 1, per_page: 3, format: 'json' + response.should be_success + json = JSON.parse(response.body) + json.should have(3).items + + get :details, review_summary_id:summaries[0].id, page: 3, per_page: 3, format: 'json' + response.should be_success + json = JSON.parse(response.body) + json.should have(1).items + end + end +end