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

Add Reviews to Subjects #597

Open
wants to merge 7 commits into
base: add-shoulda-matchers-gem
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions app/assets/stylesheets/_rating.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
.rating-container {
display: flex;
justify-content: space-between;
align-items: center;
width: 100%;
gap: 12px;
}

.rating-item {
display: flex;
align-items: center;
}

.interactive-star {
transition: transform 0.25s ease;

&:hover {
transform: scale(1.3);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it easy/worth it highlighting all the stars to the left of the one that is on hover?

}
}

.login-container {
display: flex;
gap: 4px;
align-items: center;
}
28 changes: 28 additions & 0 deletions app/controllers/reviews_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
class ReviewsController < ApplicationController
before_action :authenticate_user!
before_action :set_review, only: [:update, :destroy]

def create
@review = current_user.reviews.create!(subject_id: params[:subject_id], rating: params[:rating])

redirect_to subject_path(@review.subject)
end

def update
@review.update!(rating: params[:rating])

redirect_to subject_path(@review.subject)
end
Comment on lines +5 to +15
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you think about combining these two actions and doing an "update or create"?


def destroy
@review.destroy!

redirect_to subject_path(@review.subject)
end

private

def set_review
@review = current_user.reviews.find(params[:id])
end
end
2 changes: 2 additions & 0 deletions app/controllers/subjects_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ def index
end

def show
@user_review = current_user.reviews.find_by(subject:) if current_user

respond_to do |format|
format.html { subject }
end
Expand Down
16 changes: 16 additions & 0 deletions app/models/review.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
class Review < ApplicationRecord
belongs_to :user
belongs_to :subject

validates :user_id, uniqueness: { scope: :subject_id, message: "You can only review a subject once." }
validates :rating, presence: true, inclusion: { in: 1..5 }

after_commit :update_subject_rating, if: :saved_change_to_rating?
after_destroy :update_subject_rating

private

def update_subject_rating
subject.update_rating
end
end
5 changes: 5 additions & 0 deletions app/models/subject.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ class Subject < ApplicationRecord
has_one :course, -> { where is_exam: false }, class_name: 'Approvable', dependent: :destroy, inverse_of: :subject
has_one :exam, -> { where is_exam: true }, class_name: 'Approvable', dependent: :destroy, inverse_of: :subject
belongs_to :group, class_name: 'SubjectGroup', optional: true
has_many :reviews, dependent: :destroy

validates :name, presence: true
validates :credits, presence: true
Expand Down Expand Up @@ -47,5 +48,9 @@ def hidden_by_default?
revalid? || inactive? || outside_montevideo? || extension_module?
end

def update_rating
update(average_rating: reviews.average(:rating))
end

delegate :available?, to: :course
end
2 changes: 2 additions & 0 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ class User < ApplicationRecord

serialize :approvals, type: Array, coder: YAML

has_many :reviews, dependent: :destroy

def self.from_omniauth(auth, cookie)
# check that user with same email exists
existing_user = User.find_by(email: auth.info.email)
Expand Down
33 changes: 33 additions & 0 deletions app/views/subjects/_rating.html.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
<label class="mdc-deprecated-list-item">
Puntuación:
<div class="rating-container">
<div class="rating-item">
<button class="material-icons mdc-icon-button">star</button>
<%= @subject.average_rating.present? ? @subject.average_rating : "Sin calificar" %>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<%= @subject.average_rating.present? ? @subject.average_rating : "Sin calificar" %>
<%= @subject.average_rating || "Sin calificar" %>

</div>
<% if current_user %>
<div class="rating-item">
<% for value in 1..5 %>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<% for value in 1..5 %>
<% 1.upto(5).each |value| %>

<% filled = @user_review.present? && @user_review.rating >= value %>
<% selected = @user_review.present? && @user_review.rating == value %>
<% url = selected || @user_review.present? ? review_path(@user_review) : reviews_path %>
<% method = selected ? :delete : (@user_review.present? ? :put : :post) %>
<%= form_with(url:, method:, local: true) do |f| %>
<%= f.hidden_field :subject_id, value: @subject.id %>
<%= f.hidden_field :rating, value: %>
<%= f.button filled ? 'star' : 'star_outline', class: "material-icons mdc-icon-button interactive-star" %>
<% end %>
<% end %>
</div>
<% else %>
<%= link_to new_user_session_path do %>
<div class="login-container">
Inicia sesión para calificar
<span class="material-symbols-outlined">
login
</span>
</div>
<% end %>
Comment on lines +23 to +30
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if we show the same stars, but when an unauthenticated user clicks them we redirect them to log in?

<% end %>
</div>
</label>
2 changes: 2 additions & 0 deletions app/views/subjects/show.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
Grupo: Desconocido
<% end %>
</label>

<%= render 'rating' %>
</div>
<hr class="mdc-deprecated-list-divider">

Expand Down
1 change: 1 addition & 0 deletions config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,4 +29,5 @@
resource :user_onboardings, only: :update

resources :current_optional_subjects, only: :index
resources :reviews, only: [:create, :update, :destroy]
end
13 changes: 13 additions & 0 deletions db/migrate/20240305231624_create_reviews.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
class CreateReviews < ActiveRecord::Migration[7.1]
def change
create_table :reviews do |t|
t.references :user, null: false, foreign_key: true
t.references :subject, null: false, foreign_key: true
t.integer :rating
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we make this not null?


t.timestamps
end

add_index :reviews, [:user_id, :subject_id], unique: true
end
end
5 changes: 5 additions & 0 deletions db/migrate/20240316053425_add_average_rating_to_subjects.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class AddAverageRatingToSubjects < ActiveRecord::Migration[7.1]
def change
add_column :subjects, :average_rating, :float
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm ok with adding this, but i wonder if we should start with calculating it on the fly

end
end
17 changes: 16 additions & 1 deletion db/schema.rb

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

60 changes: 60 additions & 0 deletions spec/controllers/reviews_controller_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
require 'rails_helper'

RSpec.describe ReviewsController, type: :request do
let(:user) { create(:user) }
let(:subject) { create(:subject) }
let(:review) { create(:review, user: user, subject: subject) }

before do
# https://github.com/heartcombo/devise/issues/5705
Rails.application.reload_routes_unless_loaded
end

before do
sign_in user
end

describe 'POST #create' do
it 'creates a new review' do
expect {
post reviews_path, params: { subject_id: subject.id, rating: 5 }
}.to change(Review, :count).by(1)
end

it 'redirects to the subject page' do
post reviews_path, params: { subject_id: subject.id, rating: 5 }

expect(response).to redirect_to(subject_path(subject))
end
end

describe 'PATCH #update' do
it 'updates the review' do
patch review_path(review), params: { rating: 4 }

expect(review.reload.rating).to eq(4)
end

it 'redirects to the subject page' do
patch review_path(review), params: { rating: 4 }

expect(response).to redirect_to(subject_path(review.subject))
end
end

describe 'DELETE #destroy' do
it 'deletes the review' do
review

expect {
delete review_path(review)
}.to change(Review, :count).by(-1)
end

it 'redirects to the subject page' do
delete review_path(review)

expect(response).to redirect_to(subject_path(review.subject))
end
end
end
7 changes: 7 additions & 0 deletions spec/factories/reviews.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
FactoryBot.define do
factory :review do
user
subject
rating { 3 }
end
end
49 changes: 49 additions & 0 deletions spec/models/review_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
require 'rails_helper'

RSpec.describe Review, type: :model do
describe 'associations' do
it { is_expected.to belong_to(:user) }
it { is_expected.to belong_to(:subject) }
end

describe 'validations' do
subject { create :review }

it {
is_expected.to validate_uniqueness_of(:user_id)
.scoped_to(:subject_id)
.with_message("You can only review a subject once.")
}
it { is_expected.to validate_presence_of(:rating) }
it { is_expected.to validate_inclusion_of(:rating).in_range(1..5) }
end

describe 'callbacks' do
context 'calls update_rating on subject after commit' do
let(:review) { build :review }

it 'calls update_rating on subject after create' do
expect(review.subject).to receive(:update_rating)
review.save!
end

it 'calls update_rating on subject after update' do
review.save!
expect(review.subject).to receive(:update_rating)
review.update!(rating: 5)
end

it 'calls update_rating on subject after destroy' do
review.save!
expect(review.reload.subject).to receive(:update_rating)
review.destroy!
end

it 'does not call update_rating on subject if rating was not changed' do
review.save!
expect(review.subject).not_to receive(:update_rating)
review.update!(rating: review.rating)
end
end
end
end
21 changes: 21 additions & 0 deletions spec/models/subject_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -119,4 +119,25 @@
expect(Subject.current_semester_optionals).to eq([s1])
end
end

describe '#update_rating' do
let(:subject) { create :subject }

context 'when there are no reviews' do
it 'updates average rating' do
subject.update_rating
expect(subject.average_rating).to eq(nil)
end
end

context 'when there are reviews' do
let!(:review) { create :review, subject: subject, rating: 5 }
let!(:another_review) { create :review, subject: subject, rating: 3 }

it 'updates average rating' do
subject.update_rating
expect(subject.average_rating).to eq(4)
end
end
end
end
2 changes: 2 additions & 0 deletions spec/rails_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@
config.filter_rails_from_backtrace!
# arbitrary gems may also be filtered via:
# config.filter_gems_from_backtrace("gem name")

config.include Devise::Test::IntegrationHelpers, type: :request
end

Shoulda::Matchers.configure do |config|
Expand Down