From 91a8b4ab9cd6c601e98d23e9e316049feea6cbbd Mon Sep 17 00:00:00 2001 From: Steven Miers Date: Mon, 20 Jul 2015 11:01:08 -0500 Subject: [PATCH 01/12] VRFS-3316 : Reviews/Ratings * schemas for reviews and review_summaries * migrations * models * validations * initial specs to verify creation of reviews and review_summaries with a jamtrack and subsequently, various validations. --- db/manifest | 3 +- db/up/reviews.sql | 23 ++++++ ruby/lib/jam_ruby.rb | 2 + ruby/lib/jam_ruby/models/review.rb | 13 ++++ ruby/lib/jam_ruby/models/review_summary.rb | 12 +++ ruby/lib/jam_ruby/models/user.rb | 3 + ruby/spec/jam_ruby/models/review_spec.rb | 86 ++++++++++++++++++++++ 7 files changed, 141 insertions(+), 1 deletion(-) create mode 100644 db/up/reviews.sql create mode 100644 ruby/lib/jam_ruby/models/review.rb create mode 100644 ruby/lib/jam_ruby/models/review_summary.rb create mode 100644 ruby/spec/jam_ruby/models/review_spec.rb diff --git a/db/manifest b/db/manifest index bd6ca8ce6..d0f27852b 100755 --- a/db/manifest +++ b/db/manifest @@ -296,4 +296,5 @@ add_description_to_perf_samples.sql alter_genre_player_unique_constraint.sql musician_search.sql enhance_band_profile.sql -alter_band_profile_rate_defaults.sql \ No newline at end of file +alter_band_profile_rate_defaults.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..50ae9df5f --- /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(8000), + 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 82484997e..1069aa184 100755 --- a/ruby/lib/jam_ruby.rb +++ b/ruby/lib/jam_ruby.rb @@ -111,6 +111,8 @@ require "jam_ruby/models/machine_fingerprint" require "jam_ruby/models/machine_extra" 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..454e03943 --- /dev/null +++ b/ruby/lib/jam_ruby/models/review.rb @@ -0,0 +1,13 @@ +module JamRuby + class Review < ActiveRecord::Base + attr_accessible :target, :rating, :description, :user + 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" + + validates :rating, presence:true, numericality: {only_integer: true, minimum:1, maximum:5} + validates :target, presence:true + validates :user, presence:true + + 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..77e25d0e7 --- /dev/null +++ b/ruby/lib/jam_ruby/models/review_summary.rb @@ -0,0 +1,12 @@ +module JamRuby + class ReviewSummary < ActiveRecord::Base + attr_accessible :target, :target_type, :avg_rating, :wilson_score, :review_count + belongs_to :target, polymorphic: true + belongs_to :user, foreign_key: 'user_id', class_name: "JamRuby::User" + + 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, presence:true + 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 217c73339..5e4e2ad22 100644 --- a/ruby/lib/jam_ruby/models/user.rb +++ b/ruby/lib/jam_ruby/models/user.rb @@ -45,6 +45,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/jam_ruby/models/review_spec.rb b/ruby/spec/jam_ruby/models/review_spec.rb new file mode 100644 index 000000000..b8dd67f4d --- /dev/null +++ b/ruby/spec/jam_ruby/models/review_spec.rb @@ -0,0 +1,86 @@ +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].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 + end + + context "validates review summary" do + it "blank target" do + review_summary = ReviewSummary.create() + review_summary.valid?.should be_false + review_summary.errors[:target].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) + puts "complete:: #{review_summary.errors.inspect}" + review_summary.valid?.should be_true + 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 From 0beb386beadcc366159a964564218fa20243ff97 Mon Sep 17 00:00:00 2001 From: Steven Miers Date: Mon, 20 Jul 2015 11:29:19 -0500 Subject: [PATCH 02/12] VRFS-3316 : Review Uniqueness validation and spec. --- ruby/lib/jam_ruby/models/review.rb | 7 +++++++ ruby/spec/jam_ruby/models/review_spec.rb | 10 +++++++++- 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/ruby/lib/jam_ruby/models/review.rb b/ruby/lib/jam_ruby/models/review.rb index 454e03943..bdddb6d73 100644 --- a/ruby/lib/jam_ruby/models/review.rb +++ b/ruby/lib/jam_ruby/models/review.rb @@ -6,8 +6,15 @@ module JamRuby belongs_to :deleted_by_user, foreign_key: 'deleted_by_user_id', class_name: "JamRuby::User" validates :rating, presence:true, numericality: {only_integer: true, minimum:1, maximum:5} + validates :target, presence:true validates :user, presence:true + validates :target_id, uniqueness: {scope: :user_id, message: "There is already a review for this User and Target."} + # # @options - can contain values: + # # * target_id (optional) + # def reduce(options) + # arel = Review.where("deleted_at=?", nil) + # end end end \ No newline at end of file diff --git a/ruby/spec/jam_ruby/models/review_spec.rb b/ruby/spec/jam_ruby/models/review_spec.rb index b8dd67f4d..622b688aa 100644 --- a/ruby/spec/jam_ruby/models/review_spec.rb +++ b/ruby/spec/jam_ruby/models/review_spec.rb @@ -35,9 +35,17 @@ describe Review do end it "complete" do - review = Review.create(target:target, rating:3, user:@user) + 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 end context "validates review summary" do From 4b52d97a64cc3d8ff8acf34bea153d367eeef041 Mon Sep 17 00:00:00 2001 From: Steven Miers Date: Mon, 20 Jul 2015 11:35:16 -0500 Subject: [PATCH 03/12] VRFS-3316 : Review summary uniqueness validation and spec. --- ruby/lib/jam_ruby/models/review_summary.rb | 4 ++-- ruby/spec/jam_ruby/models/review_spec.rb | 8 ++++++++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/ruby/lib/jam_ruby/models/review_summary.rb b/ruby/lib/jam_ruby/models/review_summary.rb index 77e25d0e7..6e0ef6b11 100644 --- a/ruby/lib/jam_ruby/models/review_summary.rb +++ b/ruby/lib/jam_ruby/models/review_summary.rb @@ -2,11 +2,11 @@ module JamRuby class ReviewSummary < ActiveRecord::Base attr_accessible :target, :target_type, :avg_rating, :wilson_score, :review_count belongs_to :target, polymorphic: true - belongs_to :user, foreign_key: 'user_id', class_name: "JamRuby::User" - + 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, presence:true + validates :target_id, uniqueness:true end end \ No newline at end of file diff --git a/ruby/spec/jam_ruby/models/review_spec.rb b/ruby/spec/jam_ruby/models/review_spec.rb index 622b688aa..e9fe640f9 100644 --- a/ruby/spec/jam_ruby/models/review_spec.rb +++ b/ruby/spec/jam_ruby/models/review_spec.rb @@ -83,6 +83,14 @@ describe Review do puts "complete:: #{review_summary.errors.inspect}" 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 end end From 2c594bf7834b41332adf2d91df31cf86919c9843 Mon Sep 17 00:00:00 2001 From: Steven Miers Date: Mon, 20 Jul 2015 16:49:22 -0500 Subject: [PATCH 04/12] VRFS-3316 : Reduce method to roll up reviews * Creates review_summaries * Calculate and store wilson score * Unit tests --- ruby/lib/jam_ruby/models/review.rb | 36 ++++++++++++++++++---- ruby/lib/jam_ruby/models/review_summary.rb | 7 +++-- ruby/spec/factories.rb | 11 +++++++ ruby/spec/jam_ruby/models/review_spec.rb | 28 ++++++++++++++++- 4 files changed, 72 insertions(+), 10 deletions(-) diff --git a/ruby/lib/jam_ruby/models/review.rb b/ruby/lib/jam_ruby/models/review.rb index bdddb6d73..4e7726acc 100644 --- a/ruby/lib/jam_ruby/models/review.rb +++ b/ruby/lib/jam_ruby/models/review.rb @@ -10,11 +10,35 @@ module JamRuby validates :target, presence:true validates :user, presence:true validates :target_id, uniqueness: {scope: :user_id, message: "There is already a review for this User and Target."} - - # # @options - can contain values: - # # * target_id (optional) - # def reduce(options) - # arel = Review.where("deleted_at=?", nil) - # end + + class << self + # Create review_summary records by grouping reviews + def reduce() + 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").group("target_type, target_id") + .each do |r| + #puts "Reducing reviews: #{r.inspect} #{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: ci_lower_bound(r.pos_count, r.review_count), + review_count: r.review_count + ) + end # each + end # transaction + end # reduce + + def 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 + + end # self 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 index 6e0ef6b11..94a21e901 100644 --- a/ruby/lib/jam_ruby/models/review_summary.rb +++ b/ruby/lib/jam_ruby/models/review_summary.rb @@ -1,12 +1,13 @@ module JamRuby class ReviewSummary < ActiveRecord::Base - attr_accessible :target, :target_type, :avg_rating, :wilson_score, :review_count + 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, presence:true - validates :target_id, uniqueness:true + validates :target_id, presence:true, uniqueness:true + + end end \ No newline at end of file diff --git a/ruby/spec/factories.rb b/ruby/spec/factories.rb index 90cd48c44..893a09c17 100644 --- a/ruby/spec/factories.rb +++ b/ruby/spec/factories.rb @@ -139,6 +139,17 @@ FactoryGirl.define do end end + factory :review, :class => JamRuby::Review do + sequence(:name) { |n| "Band" } + biography "My Biography" + city "Apex" + state "NC" + country "US" + before(:create) { |review| + review.genres << Genre.first + } + 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 index e9fe640f9..dcdced01b 100644 --- a/ruby/spec/jam_ruby/models/review_spec.rb +++ b/ruby/spec/jam_ruby/models/review_spec.rb @@ -46,8 +46,33 @@ describe Review do review2 = Review.create(target:target, rating:3, user:@user) review2.valid?.should be_false end - 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.count.should eq(2) + ReviewSummary.count.should eq(0) + Review.reduce() + ReviewSummary.count.should eq(1) + ReviewSummary.first.avg_rating.should eq(4.0) + + puts "ORIG: #{ReviewSummary.all.inspect}" + ws_orig = ReviewSummary.first.wilson_score + avg_orig = ReviewSummary.first.avg_rating + + 5.times {Review.create(target:target, rating:5, user:FactoryGirl.create(:user))} + Review.reduce() + + ReviewSummary.first.wilson_score.should > ws_orig + ReviewSummary.first.avg_rating.should > avg_orig + + puts "ALL: #{ReviewSummary.all.inspect}" + end + end # context + context "validates review summary" do it "blank target" do review_summary = ReviewSummary.create() @@ -99,4 +124,5 @@ describe Review do it_behaves_like :review, @jam_track, "jam_track" end + end \ No newline at end of file From 74b8c81a8afabf6ea2df56a9872014299791502d Mon Sep 17 00:00:00 2001 From: Steven Miers Date: Mon, 20 Jul 2015 19:11:52 -0500 Subject: [PATCH 05/12] VRFS-3316 : Review query method and spec to verify. --- ruby/lib/jam_ruby/models/review.rb | 2 +- ruby/lib/jam_ruby/models/review_summary.rb | 29 ++++++++++++ ruby/spec/jam_ruby/models/review_spec.rb | 53 +++++++++++++++++++++- 3 files changed, 81 insertions(+), 3 deletions(-) diff --git a/ruby/lib/jam_ruby/models/review.rb b/ruby/lib/jam_ruby/models/review.rb index 4e7726acc..d7e6c07d1 100644 --- a/ruby/lib/jam_ruby/models/review.rb +++ b/ruby/lib/jam_ruby/models/review.rb @@ -12,7 +12,7 @@ module JamRuby validates :target_id, uniqueness: {scope: :user_id, message: "There is already a review for this User and Target."} class << self - # Create review_summary records by grouping reviews + # Create review_summary records by grouping reviews def reduce() ReviewSummary.transaction do ReviewSummary.destroy_all diff --git a/ruby/lib/jam_ruby/models/review_summary.rb b/ruby/lib/jam_ruby/models/review_summary.rb index 94a21e901..f66e3aacd 100644 --- a/ruby/lib/jam_ruby/models/review_summary.rb +++ b/ruby/lib/jam_ruby/models/review_summary.rb @@ -8,6 +8,35 @@ module JamRuby 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={}) + 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/spec/jam_ruby/models/review_spec.rb b/ruby/spec/jam_ruby/models/review_spec.rb index dcdced01b..656babb49 100644 --- a/ruby/spec/jam_ruby/models/review_spec.rb +++ b/ruby/spec/jam_ruby/models/review_spec.rb @@ -54,6 +54,8 @@ describe Review do review2 = Review.create(target:target, rating:5, user:FactoryGirl.create(:user)) review2.valid?.should be_true Review.count.should eq(2) + + # Reduce and check: ReviewSummary.count.should eq(0) Review.reduce() ReviewSummary.count.should eq(1) @@ -63,6 +65,7 @@ describe Review do 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.reduce() @@ -72,12 +75,12 @@ describe Review do puts "ALL: #{ReviewSummary.all.inspect}" 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].should == ["can't be blank"] + review_summary.errors[:target_id].should == ["can't be blank"] end it "no rating" do @@ -116,6 +119,52 @@ describe Review do 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.count.should eq(2) + + # Reduce and check: + ReviewSummary.count.should eq(0) + Review.reduce() + ReviewSummary.count.should eq(1) + ReviewSummary.first.avg_rating.should eq(4.0) + + puts "ORIG: #{ReviewSummary.all.inspect}" + 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.reduce() + ReviewSummary.count.should eq(1) + 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.reduce() + summaries = ReviewSummary.index() + summaries.count.should eq(2) + summaries[0].wilson_score.should > summaries[1].wilson_score + + summaries = ReviewSummary.index(target_id: target2) + puts "MULTIPLE TARGET ALL: #{summaries.inspect}" + summaries.count.should eq(1) + summaries[0].target_id.should eq(target2.id) + + summaries = ReviewSummary.index(target_type: "JamRuby::JamTrack") + summaries.count.should eq(2) + + summaries = ReviewSummary.index(minimum_reviews: 6) + summaries.count.should eq(1) + end end end From bd4b3380c26efabcd7019761fba8fc8b39bbd5f9 Mon Sep 17 00:00:00 2001 From: Steven Miers Date: Mon, 20 Jul 2015 20:33:20 -0500 Subject: [PATCH 06/12] VRFS-3316 : Review query method and spec to verify. --- ruby/lib/jam_ruby/models/review.rb | 40 ++++++++++++++++++------ ruby/spec/jam_ruby/models/review_spec.rb | 27 +++++++++------- 2 files changed, 46 insertions(+), 21 deletions(-) diff --git a/ruby/lib/jam_ruby/models/review.rb b/ruby/lib/jam_ruby/models/review.rb index d7e6c07d1..41b589004 100644 --- a/ruby/lib/jam_ruby/models/review.rb +++ b/ruby/lib/jam_ruby/models/review.rb @@ -12,20 +12,40 @@ module JamRuby validates :target_id, uniqueness: {scope: :user_id, message: "There is already a review for this User and Target."} class << self + def index(options={}) + if(options.key?(:include_deleted)) + arel = Review.select("*") + else + arel = Review.where("deleted_at IS NULL") + 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 reduce() 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").group("target_type, target_id") - .each do |r| - #puts "Reducing reviews: #{r.inspect} #{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: ci_lower_bound(r.pos_count, r.review_count), - review_count: r.review_count - ) + 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 # each end # transaction end # reduce diff --git a/ruby/spec/jam_ruby/models/review_spec.rb b/ruby/spec/jam_ruby/models/review_spec.rb index 656babb49..ace9d2dc1 100644 --- a/ruby/spec/jam_ruby/models/review_spec.rb +++ b/ruby/spec/jam_ruby/models/review_spec.rb @@ -53,12 +53,13 @@ describe Review do review2 = Review.create(target:target, rating:5, user:FactoryGirl.create(:user)) review2.valid?.should be_true - Review.count.should eq(2) + Review.should have(2).items + Review.index.should have(2).items # Reduce and check: - ReviewSummary.count.should eq(0) + ReviewSummary.should have(0).items Review.reduce() - ReviewSummary.count.should eq(1) + ReviewSummary.should have(1).items ReviewSummary.first.avg_rating.should eq(4.0) puts "ORIG: #{ReviewSummary.all.inspect}" @@ -67,6 +68,8 @@ describe Review do # Create some more and verify: 5.times {Review.create(target:target, rating:5, user:FactoryGirl.create(:user))} + Review.index.should have(7).items + Review.reduce() ReviewSummary.first.wilson_score.should > ws_orig @@ -125,12 +128,12 @@ describe Review do review.valid?.should be_true review2 = Review.create(target:target, rating:5, user:FactoryGirl.create(:user)) review2.valid?.should be_true - Review.count.should eq(2) + Review.should have(2).items # Reduce and check: - ReviewSummary.count.should eq(0) + ReviewSummary.should have(0).items Review.reduce() - ReviewSummary.count.should eq(1) + ReviewSummary.should have(1).items ReviewSummary.first.avg_rating.should eq(4.0) puts "ORIG: #{ReviewSummary.all.inspect}" @@ -141,7 +144,7 @@ describe Review do # Create some more and verify: 5.times {Review.create(target:target, rating:5, user:FactoryGirl.create(:user))} Review.reduce() - ReviewSummary.count.should eq(1) + ReviewSummary.should have(1).items ReviewSummary.first.wilson_score.should > ws_orig ReviewSummary.first.avg_rating.should > avg_orig @@ -149,21 +152,23 @@ describe Review do # 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 Review.reduce() summaries = ReviewSummary.index() - summaries.count.should eq(2) + summaries.should have(2).items summaries[0].wilson_score.should > summaries[1].wilson_score summaries = ReviewSummary.index(target_id: target2) puts "MULTIPLE TARGET ALL: #{summaries.inspect}" - summaries.count.should eq(1) + summaries.should have(1).items summaries[0].target_id.should eq(target2.id) summaries = ReviewSummary.index(target_type: "JamRuby::JamTrack") - summaries.count.should eq(2) + summaries.should have(2).items summaries = ReviewSummary.index(minimum_reviews: 6) - summaries.count.should eq(1) + summaries.should have(1).items end end end From 1262dbfcc6847e024f855ac57c0581a2972b00f4 Mon Sep 17 00:00:00 2001 From: Steven Miers Date: Tue, 21 Jul 2015 14:47:35 -0500 Subject: [PATCH 07/12] VRFS-3316 : Reviews : routes and api controller * Expose review query methods as an API/rest interface. * Required updates to model * Spec to verify --- ruby/lib/jam_ruby/models/review.rb | 4 +- ruby/lib/jam_ruby/models/review_summary.rb | 1 + ruby/spec/factories.rb | 13 +- ruby/spec/jam_ruby/models/review_spec.rb | 2 +- web/app/controllers/api_reviews_controller.rb | 63 +++++++++ web/config/routes.rb | 6 + .../api_reviews_controller_spec.rb | 122 ++++++++++++++++++ 7 files changed, 196 insertions(+), 15 deletions(-) create mode 100644 web/app/controllers/api_reviews_controller.rb create mode 100644 web/spec/controllers/api_reviews_controller_spec.rb diff --git a/ruby/lib/jam_ruby/models/review.rb b/ruby/lib/jam_ruby/models/review.rb index 41b589004..4c1031b3d 100644 --- a/ruby/lib/jam_ruby/models/review.rb +++ b/ruby/lib/jam_ruby/models/review.rb @@ -1,6 +1,6 @@ module JamRuby class Review < ActiveRecord::Base - attr_accessible :target, :rating, :description, :user + 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" @@ -8,7 +8,7 @@ module JamRuby validates :rating, presence:true, numericality: {only_integer: true, minimum:1, maximum:5} validates :target, presence:true - validates :user, 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."} class << self diff --git a/ruby/lib/jam_ruby/models/review_summary.rb b/ruby/lib/jam_ruby/models/review_summary.rb index f66e3aacd..fb3833f5d 100644 --- a/ruby/lib/jam_ruby/models/review_summary.rb +++ b/ruby/lib/jam_ruby/models/review_summary.rb @@ -17,6 +17,7 @@ module JamRuby # * 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 diff --git a/ruby/spec/factories.rb b/ruby/spec/factories.rb index 893a09c17..bf45bdc20 100644 --- a/ruby/spec/factories.rb +++ b/ruby/spec/factories.rb @@ -138,18 +138,7 @@ FactoryGirl.define do end end end - - factory :review, :class => JamRuby::Review do - sequence(:name) { |n| "Band" } - biography "My Biography" - city "Apex" - state "NC" - country "US" - before(:create) { |review| - review.genres << Genre.first - } - 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 index ace9d2dc1..2b62bba89 100644 --- a/ruby/spec/jam_ruby/models/review_spec.rb +++ b/ruby/spec/jam_ruby/models/review_spec.rb @@ -31,7 +31,7 @@ describe Review do it "no user" do review = Review.create(target:target, rating:3) review.valid?.should be_false - review.errors[:user].should include("can't be blank") + review.errors[:user_id].should include("can't be blank") end it "complete" do diff --git a/web/app/controllers/api_reviews_controller.rb b/web/app/controllers/api_reviews_controller.rb new file mode 100644 index 000000000..83f8fed45 --- /dev/null +++ b/web/app/controllers/api_reviews_controller.rb @@ -0,0 +1,63 @@ +require 'sanitize' +class ApiReviewsController < ApiController + + before_filter :api_signed_in_user, :except => [:index] + #before_filter :auth_user, :only => [:create, :update, :delete] + before_filter :lookup_review_summary, :only => [:details] + before_filter :lookup_review, :only => [:update, :delete, :show] + + respond_to :json + + 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 + + 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 + + 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 + + def delete + @review.deleted_at = Time.now() + @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 + puts "FFFound: #{@review}" + end +end diff --git a/web/config/routes.rb b/web/config/routes.rb index 2281f6937..34711060e 100644 --- a/web/config/routes.rb +++ b/web/config/routes.rb @@ -288,6 +288,12 @@ SampleApp::Application.routes.draw do match '/users/complete/:signup_token' => 'api_users#complete', as: 'complete', via: 'post' 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..d129a45fb --- /dev/null +++ b/web/spec/controllers/api_reviews_controller_spec.rb @@ -0,0 +1,122 @@ +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 + 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 + json = JSON.parse(response.body) + puts "response.inspect: #{JSON.pretty_generate(json)}" + + 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).items + puts "response.inspect: #{JSON.pretty_generate(json)}" + + 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 + + puts "details: #{JSON.pretty_generate(json)}" + 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 From 2d4ce4cf3c24ed7fcc18e5ffe4e612a0dc9a1cff Mon Sep 17 00:00:00 2001 From: Steven Miers Date: Tue, 21 Jul 2015 16:55:40 -0500 Subject: [PATCH 08/12] VRFS-3316 : Review cleanup, new tests --- web/app/controllers/api_reviews_controller.rb | 9 ++++++--- .../api_reviews_controller_spec.rb | 20 +++++++++---------- 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/web/app/controllers/api_reviews_controller.rb b/web/app/controllers/api_reviews_controller.rb index 83f8fed45..a7ace92af 100644 --- a/web/app/controllers/api_reviews_controller.rb +++ b/web/app/controllers/api_reviews_controller.rb @@ -1,13 +1,12 @@ require 'sanitize' class ApiReviewsController < ApiController - before_filter :api_signed_in_user, :except => [:index] - #before_filter :auth_user, :only => [:create, :update, :delete] 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]) @@ -26,12 +25,14 @@ class ApiReviewsController < ApiController 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? @@ -42,8 +43,10 @@ class ApiReviewsController < ApiController 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 @@ -58,6 +61,6 @@ private arel = Review.where("id=?", params[:id]) arel = arel.where("user_id=?", current_user) unless current_user.admin @review = arel.first - puts "FFFound: #{@review}" + raise ActiveRecord::RecordNotFound, "Couldn't find review matching #{arel}" if @review.nil? end end diff --git a/web/spec/controllers/api_reviews_controller_spec.rb b/web/spec/controllers/api_reviews_controller_spec.rb index d129a45fb..29a670c28 100644 --- a/web/spec/controllers/api_reviews_controller_spec.rb +++ b/web/spec/controllers/api_reviews_controller_spec.rb @@ -2,8 +2,6 @@ require 'spec_helper' describe ApiReviewsController do render_views - - before(:all) do @logged_in_user = FactoryGirl.create(:user) end @@ -35,6 +33,7 @@ describe ApiReviewsController 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 @@ -42,12 +41,18 @@ describe ApiReviewsController do @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 @@ -68,18 +73,14 @@ describe ApiReviewsController do get :index, format: 'json' response.should be_success json = JSON.parse(response.body) - json.should have(0).items - json = JSON.parse(response.body) - puts "response.inspect: #{JSON.pretty_generate(json)}" + 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).items - puts "response.inspect: #{JSON.pretty_generate(json)}" - + json.should have(2).item end it "details" do @@ -87,7 +88,6 @@ describe ApiReviewsController do 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 @@ -98,8 +98,6 @@ describe ApiReviewsController do response.should be_success json = JSON.parse(response.body) json.should have(5).items - - puts "details: #{JSON.pretty_generate(json)}" end it "paginates details" do From 46cd9c47988afdf46f8f7b91294bd60c81a6c560 Mon Sep 17 00:00:00 2001 From: Steven Miers Date: Tue, 21 Jul 2015 18:10:45 -0500 Subject: [PATCH 09/12] Remove stray debug output. --- ruby/spec/jam_ruby/models/review_spec.rb | 5 ----- 1 file changed, 5 deletions(-) diff --git a/ruby/spec/jam_ruby/models/review_spec.rb b/ruby/spec/jam_ruby/models/review_spec.rb index 2b62bba89..a8cc13cc7 100644 --- a/ruby/spec/jam_ruby/models/review_spec.rb +++ b/ruby/spec/jam_ruby/models/review_spec.rb @@ -62,7 +62,6 @@ describe Review do ReviewSummary.should have(1).items ReviewSummary.first.avg_rating.should eq(4.0) - puts "ORIG: #{ReviewSummary.all.inspect}" ws_orig = ReviewSummary.first.wilson_score avg_orig = ReviewSummary.first.avg_rating @@ -75,7 +74,6 @@ describe Review do ReviewSummary.first.wilson_score.should > ws_orig ReviewSummary.first.avg_rating.should > avg_orig - puts "ALL: #{ReviewSummary.all.inspect}" end end # context @@ -111,7 +109,6 @@ describe Review do it "complete" do review_summary = ReviewSummary.create(target:target, avg_rating:3.2, wilson_score:0.95, review_count: 15) - puts "complete:: #{review_summary.errors.inspect}" review_summary.valid?.should be_true end @@ -136,7 +133,6 @@ describe Review do ReviewSummary.should have(1).items ReviewSummary.first.avg_rating.should eq(4.0) - puts "ORIG: #{ReviewSummary.all.inspect}" ws_orig = ReviewSummary.first.wilson_score avg_orig = ReviewSummary.first.avg_rating @@ -160,7 +156,6 @@ describe Review do summaries[0].wilson_score.should > summaries[1].wilson_score summaries = ReviewSummary.index(target_id: target2) - puts "MULTIPLE TARGET ALL: #{summaries.inspect}" summaries.should have(1).items summaries[0].target_id.should eq(target2.id) From 82aa6bb24439dbeb44e9b15e92d3f9c46f2d6a7f Mon Sep 17 00:00:00 2001 From: Steven Miers Date: Tue, 21 Jul 2015 19:14:18 -0500 Subject: [PATCH 10/12] Use scopes to clean up. --- ruby/lib/jam_ruby/models/review.rb | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/ruby/lib/jam_ruby/models/review.rb b/ruby/lib/jam_ruby/models/review.rb index 4c1031b3d..cbd0648d3 100644 --- a/ruby/lib/jam_ruby/models/review.rb +++ b/ruby/lib/jam_ruby/models/review.rb @@ -5,6 +5,9 @@ module JamRuby 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 :rating, presence:true, numericality: {only_integer: true, minimum:1, maximum:5} validates :target, presence:true @@ -14,9 +17,9 @@ module JamRuby class << self def index(options={}) if(options.key?(:include_deleted)) - arel = Review.select("*") + arel = Review.all else - arel = Review.where("deleted_at IS NULL") + arel = Review.available end if(options.key?(:target_id)) From 82dd27754c0e07d0d78f3d4de94e366f3a8b0ab9 Mon Sep 17 00:00:00 2001 From: Steven Miers Date: Wed, 22 Jul 2015 02:54:10 +0000 Subject: [PATCH 11/12] Tweak --- web/README.md | 2 -- 1 file changed, 2 deletions(-) diff --git a/web/README.md b/web/README.md index 393628cf1..f2dccf0a1 100644 --- a/web/README.md +++ b/web/README.md @@ -2,5 +2,3 @@ Jasmine Javascript Unit Tests ============================= Open browser to localhost:3000/teaspoon - - From 69c05a3e0a4f793754d718b37898a4b9b06d89b5 Mon Sep 17 00:00:00 2001 From: Seth Call Date: Tue, 12 Jan 2016 21:01:39 -0600 Subject: [PATCH 12/12] * reviews updated --- db/up/reviews.sql | 2 +- ruby/lib/jam_ruby/models/review.rb | 120 ++++++++++++++--------- ruby/spec/jam_ruby/models/review_spec.rb | 10 +- 3 files changed, 75 insertions(+), 57 deletions(-) diff --git a/db/up/reviews.sql b/db/up/reviews.sql index 50ae9df5f..89a30ee32 100644 --- a/db/up/reviews.sql +++ b/db/up/reviews.sql @@ -3,7 +3,7 @@ CREATE TABLE reviews ( 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(8000), + 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, diff --git a/ruby/lib/jam_ruby/models/review.rb b/ruby/lib/jam_ruby/models/review.rb index cbd0648d3..157790e43 100644 --- a/ruby/lib/jam_ruby/models/review.rb +++ b/ruby/lib/jam_ruby/models/review.rb @@ -1,67 +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("*")} + scope :all, -> { select("*") } - validates :rating, presence:true, numericality: {only_integer: true, minimum:1, maximum:5} + 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, 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."} - class << self - def index(options={}) - if(options.key?(:include_deleted)) - arel = Review.all - else - arel = Review.available - end + after_save :reduce - 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 + def self.index(options={}) + if options.key?(:include_deleted) + arel = Review.all + else + arel = Review.available end - # Create review_summary records by grouping reviews - def reduce() - 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 # each - end # transaction - end # reduce - - def 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) + if options.key?(:target_id) + arel = arel.where("target_id=?", options[:target_id]) end - end # self + 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/spec/jam_ruby/models/review_spec.rb b/ruby/spec/jam_ruby/models/review_spec.rb index a8cc13cc7..7a5851948 100644 --- a/ruby/spec/jam_ruby/models/review_spec.rb +++ b/ruby/spec/jam_ruby/models/review_spec.rb @@ -57,8 +57,6 @@ describe Review do Review.index.should have(2).items # Reduce and check: - ReviewSummary.should have(0).items - Review.reduce() ReviewSummary.should have(1).items ReviewSummary.first.avg_rating.should eq(4.0) @@ -68,8 +66,7 @@ describe Review do # Create some more and verify: 5.times {Review.create(target:target, rating:5, user:FactoryGirl.create(:user))} Review.index.should have(7).items - - Review.reduce() + ReviewSummary.should have(1).items ReviewSummary.first.wilson_score.should > ws_orig ReviewSummary.first.avg_rating.should > avg_orig @@ -127,9 +124,6 @@ describe Review do review2.valid?.should be_true Review.should have(2).items - # Reduce and check: - ReviewSummary.should have(0).items - Review.reduce() ReviewSummary.should have(1).items ReviewSummary.first.avg_rating.should eq(4.0) @@ -139,7 +133,6 @@ describe Review do # Create some more and verify: 5.times {Review.create(target:target, rating:5, user:FactoryGirl.create(:user))} - Review.reduce() ReviewSummary.should have(1).items ReviewSummary.first.wilson_score.should > ws_orig ReviewSummary.first.avg_rating.should > avg_orig @@ -150,7 +143,6 @@ describe Review do 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 - Review.reduce() summaries = ReviewSummary.index() summaries.should have(2).items summaries[0].wilson_score.should > summaries[1].wilson_score