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

Send rejection reasons to reviews #108

Merged
merged 9 commits into from
Jul 30, 2023
Merged
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
4 changes: 2 additions & 2 deletions .env.development
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,5 @@ DATABASE_URL=postgres://localhost:5432/hotseat_dev
ENROLLMENT_NOTIFICATION_API_TOKEN=fake-token
BLAZER_USERNAME=blazer
BLAZER_PASSWORD=blazer_pass_456
VAPID_PUBLIC_KEY=BEUN6k7_zRQAMYy1fZX9tpfNb_Id9nWj_1cE5II3GmnKUDH8p855qsDohiV7ZHP-_hUNQxInUs-e0Ye-IJG7jPE=
VAPID_PRIVATE_KEY=qk0ULlxsiawKN9o5OI2oYtghrLjMV7nwvD8FjvBzhdc
VAPID_PUBLIC_KEY=BIxQD5USAbW5JQVPKD053uFongU_yVcGHu3QiSUhHBmMMN-PLfwiJuol20YyKucD2W8cysiVdZw1uCmnkH0r_aE=
VAPID_PRIVATE_KEY=8BblV-XtLUUvtGnmmnRIUtBAhOUMHdxvr48v6XgmSv4=
4 changes: 2 additions & 2 deletions .env.test
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,5 @@ AWS_DEFAULT_REGION=us-east-1
BLAZER_USERNAME=blazer
BLAZER_PASSWORD=blazer_pass_456
ENROLLMENT_NOTIFICATION_API_TOKEN=fake-token
VAPID_PUBLIC_KEY=BEUN6k7_zRQAMYy1fZX9tpfNb_Id9nWj_1cE5II3GmnKUDH8p855qsDohiV7ZHP-_hUNQxInUs-e0Ye-IJG7jPE=
VAPID_PRIVATE_KEY=qk0ULlxsiawKN9o5OI2oYtghrLjMV7nwvD8FjvBzhdc
VAPID_PUBLIC_KEY=BIxQD5USAbW5JQVPKD053uFongU_yVcGHu3QiSUhHBmMMN-PLfwiJuol20YyKucD2W8cysiVdZw1uCmnkH0r_aE=
VAPID_PRIVATE_KEY=8BblV-XtLUUvtGnmmnRIUtBAhOUMHdxvr48v6XgmSv4=
8 changes: 5 additions & 3 deletions app/controllers/admin/reviews_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -37,22 +37,24 @@ def show

class UpdateParams < T::Struct
const :id, Integer
const :status, String
const :button, String
const :rejection_reason, T.nilable(String)
end

# PATCH/PUT /admin/review/:id
sig { void }
def update
typed_params = TypedParams.extract!(UpdateParams, params)
status = typed_params.status
status = typed_params.button
rejection_reason = typed_params.rejection_reason

@review = Review.find(typed_params.id)

case status
when "approved"
@review.approve!
when "rejected"
@review.reject!
@review.reject!(rejection_reason)
when "pending"
@review.set_pending!
else
Expand Down
12 changes: 10 additions & 2 deletions app/controllers/enrollment_notifications_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,11 @@
user = User.find(typed_params.admin_test)
if user.admin?
timestamp = Time.now.to_i
EnrollmentNotification.with(timestamp:, section:, previous_enrollment_numbers: typed_params.previous_enrollment_numbers.serialize).deliver_later(user)
EnrollmentNotification.with(

Check warning on line 33 in app/controllers/enrollment_notifications_controller.rb

View check run for this annotation

Codecov / codecov/patch

app/controllers/enrollment_notifications_controller.rb#L33

Added line #L33 was not covered by tests
timestamp:,
section:,
previous_enrollment_numbers: typed_params.previous_enrollment_numbers.serialize.symbolize_keys,
).deliver_later(user)
render(json: { admin: true, notifications_sent: 1 })
else
render(json: { not_admin: true, notifications_sent: 0 })
Expand All @@ -47,7 +51,11 @@
timestamp = Time.now.to_i
subscriptions = section.relationships.where(notify: true)
subscriptions.each do |relationship|
EnrollmentNotification.with(timestamp:, section:, previous_enrollment_numbers: typed_params.previous_enrollment_numbers.serialize).deliver_later(relationship.user)
EnrollmentNotification.with(
timestamp:,
section:,
previous_enrollment_numbers: typed_params.previous_enrollment_numbers.serialize.symbolize_keys,
).deliver_later(relationship.user)
end

render(json: { notifications_sent: subscriptions.size })
Expand Down
36 changes: 0 additions & 36 deletions app/jobs/notify_user_about_approved_review_job.rb

This file was deleted.

39 changes: 0 additions & 39 deletions app/jobs/notify_user_about_rejected_review_job.rb

This file was deleted.

9 changes: 5 additions & 4 deletions app/models/review.rb
Original file line number Diff line number Diff line change
Expand Up @@ -235,24 +235,25 @@ def approve!
T.must(user).complete_referral!
T.must(user).add_notification_token
update(first_approved_at: Time.zone.now)
NotifyUserAboutApprovedReviewJob.perform_later(self)
ReviewApprovedNotification.with(review: self).deliver_later(user)
end

approved!
save!

true
end

# Reject the review.
# This method is idempotent.
sig { returns(T::Boolean) }
def reject!
sig { params(rejection_reason: T.nilable(String)).returns(T::Boolean) }
def reject!(rejection_reason = nil)
# Can't reject after a review is approved
return false if approved?
# No point in rejecting if already rejected
return false if rejected?

NotifyUserAboutRejectedReviewJob.perform_later(self)
ReviewRejectedNotification.with(review: self, rejection_reason:).deliver_later(user)
rejected!
save!

Expand Down
4 changes: 2 additions & 2 deletions app/notifications/delivery_methods/aws_text_message.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ class DeliveryMethods::AwsTextMessage < Noticed::DeliveryMethods::Base
sig { returns(User) }
attr_reader :recipient

sig { returns(EnrollmentNotification) }
sig { returns(T.any(EnrollmentNotification, ReviewApprovedNotification, ReviewRejectedNotification)) }
attr_reader :notification

sig { void }
Expand All @@ -22,7 +22,7 @@ def deliver

client = Aws::SNS::Client.new(region: "us-east-1")

if T.unsafe(Rails.env).development?
if Rails.env.development?
Rails.logger.info("Skipping sending message since we're not in production.")
else
response = client.publish({ phone_number: recipient.phone, message: notification.full_message })
Expand Down
9 changes: 3 additions & 6 deletions app/notifications/enrollment_notification.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,6 @@

class EnrollmentNotification < Noticed::Base
extend T::Sig
# include GeneratedUrlHelpers
# include Rails.application.routes.url_helpers
# include GeneratedUrlHelpers

deliver_by :database
# deliver_by :email, mailer: "UserMailer"
Expand All @@ -24,7 +21,7 @@ class EnrollmentNotification < Noticed::Base

sig { returns(String) }
def full_message
old_enrollment_status = params[:previous_enrollment_numbers]["enrollment_status"]
old_enrollment_status = params[:previous_enrollment_numbers][:enrollment_status]
section = params[:section]
new_enrollment_status = section.enrollment_status

Expand All @@ -43,7 +40,7 @@ def title

sig { returns(String) }
def body
old_enrollment_status = params[:previous_enrollment_numbers]["enrollment_status"]
old_enrollment_status = params[:previous_enrollment_numbers][:enrollment_status]
section = params[:section]
new_enrollment_status = section.enrollment_status

Expand Down Expand Up @@ -82,7 +79,7 @@ def enrollment_status

sig { returns(T::Boolean) }
def webpush_notifications?
recipient.admin?
recipient.beta_tester
end

sig { returns(T::Boolean) }
Expand Down
26 changes: 26 additions & 0 deletions app/notifications/review_approved_notification.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
# typed: strict
# frozen_string_literal: true

# To deliver this notification:
#
# ReviewRejectedNotification.with(post: @post).deliver_later(current_user)
# ReviewRejectedNotification.with(post: @post).deliver(current_user)

class ReviewApprovedNotification < Noticed::Base
extend T::Sig

deliver_by :aws_text_message, class: "DeliveryMethods::AwsTextMessage"

sig { returns(String) }
def full_message
review = T.let(params[:review], Review)
user = T.must(review.user)
section = T.must(review.section)

message = <<~MESSAGE
Your Hotseat review for #{section.course_title} was approved. You now have #{user.notification_token_count} notification tokens.
MESSAGE

message.strip
end
end
34 changes: 34 additions & 0 deletions app/notifications/review_rejected_notification.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
# typed: strict
# frozen_string_literal: true

# To deliver this notification:
#
# ReviewRejectedNotification.with(post: @post).deliver_later(current_user)
# ReviewRejectedNotification.with(post: @post).deliver(current_user)

class ReviewRejectedNotification < Noticed::Base
extend T::Sig

deliver_by :aws_text_message, class: "DeliveryMethods::AwsTextMessage"

sig { returns(String) }
def full_message
review = T.let(params[:review], Review)
rejection_reason = T.let(params[:rejection_reason], T.nilable(String))
section = T.must(review.section)

message = if rejection_reason.blank?
<<~MESSAGE
Your Hotseat review for #{section.course_title} was not approved. No worries! You can update and resubmit your review at #{edit_review_url(review)} or reach out to [email protected].
MESSAGE
else
<<~MESSAGE
Your Hotseat review for #{section.course_title} was not approved for the following reason: #{rejection_reason}

No worries! You can update and resubmit your review at #{edit_review_url(review)} or reach out to [email protected].
MESSAGE
end

message.strip
end
end
24 changes: 18 additions & 6 deletions app/views/admin/reviews/show.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,14 @@

<%= render "shared/page_header", title: "Admin: Review #{@review.id}", breadcrumbs: [{ name: "Home", link: "/" }, { name: "Admin: Reviews", link: admin_reviews_path }, { name: "Review #{@review.id}", link: admin_review_path(@review) }] %>

<div class="overflow-hidden bg-white shadow sm:rounded-lg">
<div class="overflow-hidden bg-white dark:bg-gray-900 shadow sm:rounded-lg">
<div class="px-4 py-5 sm:px-6">
<h3 class="text-base font-semibold leading-6 text-gray-900">
<h3 class="text-base font-semibold leading-6 text-gray-900 dark:text-gray-50">
<%= link_to course_instructor_path(@review.course, @review.instructor) do %>
<%= @review.course.long_title %>
<% end %>
</h3>
<p class="mt-1 max-w-2xl text-sm text-gray-500">
<p class="mt-1 max-w-2xl text-sm text-gray-500 dark:text-gray-400">
<%= link_to instructor_path(@review.instructor) do %>
<%= @review.section.instructor.full_label %>
<% end %>
Expand Down Expand Up @@ -86,11 +86,23 @@
</div>
<% end %>

<div class="flex w-full justify-center">
<%= button_to "Reject", [:admin, @review], method: :patch, params: { status: "rejected" }, class: "inline-flex items-center rounded-md border border-transparent bg-red-100 px-6 py-3 text-base font-medium text-red-700 hover:bg-red-200 focus:outline-none focus:ring-2 focus:ring-red-500 focus:ring-offset-2 mr-4" %>
<%= button_to "Approve", [:admin, @review], method: :patch, params: { status: "approved" }, class: "inline-flex items-center rounded-md border border-transparent bg-red-600 px-6 py-3 text-base font-medium text-white shadow-sm hover:bg-red-700 focus:outline-none focus:ring-2 focus:ring-red-500 focus:ring-offset-2" %>
<div class="flex flex-col w-full">
<%= form_with url: [:admin, @review], method: :patch do |form| %>
<%= form.label(:rejection_reason, "Rejection reason (sent to reviewer)") %>
<%= form.text_area(:rejection_reason, class: "block w-full rounded-md shadow-sm focus:border-red-300 focus:ring focus:ring-red-200 focus:ring-opacity-50 border-gray-300 dark:border-gray-600 bg-white dark:bg-gray-900") %>

<div class="flex justify-center mt-4">
<%= form.button("Reject", value: "rejected", class: "inline-flex items-center rounded-md border border-transparent bg-red-100 px-6 py-3 text-base font-medium text-red-700 hover:bg-red-200 focus:outline-none focus:ring-2 focus:ring-red-500 focus:ring-offset-2 mr-4") %>
<%= form.button("Approve", value: "approved", class: "inline-flex items-center rounded-md border border-transparent bg-red-600 px-6 py-3 text-base font-medium text-white shadow-sm hover:bg-red-700 focus:outline-none focus:ring-2 focus:ring-red-500 focus:ring-offset-2") %>
</div>
<% end %>
</div>

<%# <%= button_to "Reject", [:admin, @review], method: :patch, params: { status: "rejected" }, class: "inline-flex items-center rounded-md border border-transparent bg-red-100 px-6 py-3 text-base font-medium text-red-700 hover:bg-red-200 focus:outline-none focus:ring-2 focus:ring-red-500 focus:ring-offset-2 mr-4" %1> %>
<%# <%= button_to "Approve", [:admin, @review], method: :patch, params: { status: "approved" }, class: "inline-flex items-center rounded-md border border-transparent bg-red-600 px-6 py-3 text-base font-medium text-white shadow-sm hover:bg-red-700 focus:outline-none focus:ring-2 focus:ring-red-500 focus:ring-offset-2" %1> %>
<%# </div> %>
<%# </div> %>

<div class="flex w-full justify-center">
<p class="py-1 text-base text-gray-500 dark:text-gray-400">
Status: <%= @review.status %>
Expand Down
Loading
Loading