Skip to content

Commit

Permalink
Add Avenger Backlog Notifications (source-academy#932)
Browse files Browse the repository at this point in the history
* Create NotificationType Model

- Create Notifications module and NotificationType model
- Start migration for adding notifications
- No functionalities added yet only template code

* Configure Oban and Bamboo for notifications system

* Create NotificationConfig Model

* Create TimeOption Model

* Create NotificationPreference Model

* Create SentNotification Model

* Add Email Field to User Model

* fix: Fix Oban configuration

* chore: Add dependency on bamboo_phoenix for email template rendering

* feat: Implement Oban job for avenger backlog emails

* chore: Prevent browser from opening automatically when a local email is sent

* Add migrations for notification triggers and avenger backlog
notification entry

* Implement notification configuration checks

* Fix formatting errors

* fix: Fix notifications schema typos

Changes:
* Replaced is_enable to is_enabled for Notification Preferences
* Update default is_enabled to true for Notifcation Types

* fix: Fix test configurations not being applied

`import_config` must always appear at the bottom for environment
specific configurations to be applied correctly. All configurations
after this line will overwrite configurations that exists in the
environment specific ones.

* chore: remove unused controllers and views

- remove auto-generated controllers and views that are not used

* chore: Add tests for notifications

* chore: Add test for avenger backlog email

* chore: Resolve style violations

* chore: Resolve style violations

* chore: Resolve style violations

* style: fix formatting

* fix: Fix bad refactoring

* fix: Fix testing environment with Oban and Bamboo

* test: add tests for notification types

* test: add tests for time options

* chore: Update constraints and changesets for notification models

* chore: Update default behaviour for no time_option in user preference

If user preference has no time option, use the time_option from
notification_config instead. This is so that the behaviour of these
users with no preferences would always follow the default chosen by the
course admin

* style: fix formatting

* test: add tests for sent notifications

* Add tests for notifications module

* fix: Fix testing with Oban

Oban introduced changes to testing in v2.12, this commit changes the old
test configurations to the new one recommended by official docs.

* Add more tests for notifications module

* test: add tests for NotificationWorker

* style: fix formatting

* style: fix formatting

* style: fix formatting

* fix: Fix tests under notification types name constraints

* feat: Implement job for assessment submission mail

* fix: Fix assessment submission worker

* chore: Add test for assessment submission

* chore: Add migration to populate existing nus users' emails

Current nus users will have their email attribute populated based on
their username. nus users are identified from the provider attribute.

Future nus users will have their email attribute populated on creation
ideally.

* feat: implement sent_notifications

- move mailing logic to notification worker
- insert into sent_notifications when email is sent out successfully

* fix: fix tests

* style: fix formatting

* fix: fix guard clauses

- move guard clauses to prevent unnecessary querying

* fix: Fix db triggers not running for assessment submission notifications

---------

Co-authored-by: Goh Jun Yi <[email protected]>
Co-authored-by: Martin Henz <[email protected]>
  • Loading branch information
3 people authored May 25, 2023
1 parent 6837435 commit ed7f772
Show file tree
Hide file tree
Showing 53 changed files with 1,892 additions and 11 deletions.
23 changes: 19 additions & 4 deletions config/config.exs
Original file line number Diff line number Diff line change
Expand Up @@ -72,10 +72,6 @@ config :sentry,
root_source_code_path: File.cwd!(),
context_lines: 5

# Import environment specific config. This must remain at the bottom
# of this file so it overrides the configuration defined above.
import_config "#{Mix.env()}.exs"

# Configure Phoenix Swagger
config :cadet, :phoenix_swagger,
swagger_files: %{
Expand All @@ -93,3 +89,22 @@ config :guardian, Guardian.DB,
token_types: ["refresh"],
# default: 60 minute
sweep_interval: 180

config :cadet, Oban,
repo: Cadet.Repo,
plugins: [
# keep
{Oban.Plugins.Pruner, max_age: 60},
{Oban.Plugins.Cron,
crontab: [
{"@daily", Cadet.Workers.NotificationWorker,
args: %{"notification_type" => "avenger_backlog"}}
]}
],
queues: [default: 10, notifications: 1]

config :cadet, Cadet.Mailer, adapter: Bamboo.LocalAdapter

# Import environment specific config. This must remain at the bottom
# of this file so it overrides the configuration defined above.
import_config "#{Mix.env()}.exs"
2 changes: 2 additions & 0 deletions config/prod.exs
Original file line number Diff line number Diff line change
Expand Up @@ -44,3 +44,5 @@ config :logger, level: :info
config :ex_aws,
access_key_id: [:instance_role],
secret_access_key: [:instance_role]

config :cadet, Cadet.Mailer, adapter: Bamboo.SesAdapter
6 changes: 6 additions & 0 deletions config/test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -88,3 +88,9 @@ config :arc, storage: Arc.Storage.Local

if "test.secrets.exs" |> Path.expand(__DIR__) |> File.exists?(),
do: import_config("test.secrets.exs")

config :cadet, Oban,
repo: Cadet.Repo,
testing: :manual

config :cadet, Cadet.Mailer, adapter: Bamboo.TestAdapter
26 changes: 26 additions & 0 deletions lib/cadet/accounts/course_registrations.ex
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,13 @@ defmodule Cadet.Accounts.CourseRegistrations do
|> Repo.all()
end

def get_staffs(course_id) do
CourseRegistration
|> where(course_id: ^course_id)
|> where(role: :staff)
|> Repo.all()
end

def get_users(course_id, group_id) when is_ecto_id(group_id) and is_ecto_id(course_id) do
CourseRegistration
|> where([cr], cr.course_id == ^course_id)
Expand Down Expand Up @@ -200,4 +207,23 @@ defmodule Cadet.Accounts.CourseRegistrations do
{:error, changeset} -> {:error, {:bad_request, full_error_messages(changeset)}}
end
end

def get_avenger_of(student_id) when is_ecto_id(student_id) do
CourseRegistration
|> Repo.get_by(id: student_id)
|> Repo.preload(:group)
|> Map.get(:group)
|> case do
nil ->
nil

group ->
avenger_id = Map.get(group, :leader_id)

CourseRegistration
|> where([cr], cr.id == ^avenger_id)
|> preload(:user)
|> Repo.one()
end
end
end
1 change: 1 addition & 0 deletions lib/cadet/accounts/user.ex
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ defmodule Cadet.Accounts.User do
field(:username, :string)
field(:provider, :string)
field(:super_admin, :boolean)
field(:email, :string)

belongs_to(:latest_viewed_course, Course)
has_many(:courses, CourseRegistration)
Expand Down
4 changes: 3 additions & 1 deletion lib/cadet/application.ex
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@ defmodule Cadet.Application do
# Start the GuardianDB sweeper
worker(Guardian.DB.Token.SweeperServer, []),
# Start the Quantum scheduler
worker(Cadet.Jobs.Scheduler, [])
worker(Cadet.Jobs.Scheduler, []),
# Start the Oban instance
{Oban, Application.fetch_env!(:cadet, Oban)}
]

children =
Expand Down
24 changes: 22 additions & 2 deletions lib/cadet/assessments/assessments.ex
Original file line number Diff line number Diff line change
Expand Up @@ -721,11 +721,24 @@ defmodule Cadet.Assessments do
|> Repo.one()
end

def get_submission_by_id(submission_id) when is_ecto_id(submission_id) do
Submission
|> where(id: ^submission_id)
|> join(:inner, [s], a in assoc(s, :assessment))
|> preload([_, a], assessment: a)
|> Repo.one()
end

def finalise_submission(submission = %Submission{}) do
with {:status, :attempted} <- {:status, submission.status},
{:ok, updated_submission} <- update_submission_status_and_xp_bonus(submission) do
# Couple with update_submission_status_and_xp_bonus to ensure notification is sent
Notifications.write_notification_when_student_submits(submission)
# Send email notification to avenger
%{notification_type: "assessment_submission", submission_id: updated_submission.id}
|> Cadet.Workers.NotificationWorker.new()
|> Oban.insert()

# Begin autograding job
GradingJob.force_grade_individual_submission(updated_submission)

Expand Down Expand Up @@ -1151,7 +1164,8 @@ defmodule Cadet.Assessments do
{:ok, String.t()}
def all_submissions_by_grader_for_index(
grader = %CourseRegistration{course_id: course_id},
group_only \\ false
group_only \\ false,
ungraded_only \\ false
) do
show_all = not group_only

Expand All @@ -1161,6 +1175,11 @@ defmodule Cadet.Assessments do
else:
"where s.student_id in (select cr.id from course_registrations cr inner join groups g on cr.group_id = g.id where g.leader_id = $2) or s.student_id = $2"

ungraded_where =
if ungraded_only,
do: "where s.\"gradedCount\" < assts.\"questionCount\"",
else: ""

params = if show_all, do: [course_id], else: [course_id, grader.id]

# We bypass Ecto here and use a raw query to generate JSON directly from
Expand Down Expand Up @@ -1200,7 +1219,7 @@ defmodule Cadet.Assessments do
group by s.id) s
inner join
(select
a.id, to_json(a) as jsn
a.id, a."questionCount", to_json(a) as jsn
from
(select
a.id,
Expand Down Expand Up @@ -1240,6 +1259,7 @@ defmodule Cadet.Assessments do
from course_registrations cr
inner join
users u on u.id = cr.user_id) cr) unsubmitters on s.unsubmitted_by_id = unsubmitters.id
#{ungraded_where}
) q
""",
params
Expand Down
10 changes: 8 additions & 2 deletions lib/cadet/courses/courses.ex
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,12 @@ defmodule Cadet.Courses do
end
end

def get_all_course_ids do
Course
|> select([c], c.id)
|> Repo.all()
end

defp retrieve_course(course_id) when is_ecto_id(course_id) do
Course
|> where(id: ^course_id)
Expand Down Expand Up @@ -233,8 +239,8 @@ defmodule Cadet.Courses do
)
|> where(course_id: ^course_id)
|> Repo.one()} do
# It is ok to assume that user course registions already exist, as they would have been created
# in the admin_user_controller before calling this function
# It is ok to assume that user course registions already exist, as they would
# have been created in the admin_user_controller before calling this function
case role do
# If student, update his course registration
:student ->
Expand Down
40 changes: 40 additions & 0 deletions lib/cadet/email.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
defmodule Cadet.Email do
@moduledoc """
Contains methods for sending email notifications.
"""
use Bamboo.Phoenix, view: CadetWeb.EmailView
import Bamboo.Email

def avenger_backlog_email(template_file_name, avenger, ungraded_submissions) do
if is_nil(avenger.email) do
nil
else
base_email()
|> to(avenger.email)
|> assign(:avenger_name, avenger.name)
|> assign(:submissions, ungraded_submissions)
|> subject("Backlog for #{avenger.name}")
|> render("#{template_file_name}.html")
end
end

def assessment_submission_email(template_file_name, avenger, student, submission) do
if is_nil(avenger.email) do
nil
else
base_email()
|> to(avenger.email)
|> assign(:avenger_name, avenger.name)
|> assign(:student_name, student.name)
|> assign(:assessment_title, submission.assessment.title)
|> subject("New submission for #{submission.assessment.title}")
|> render("#{template_file_name}.html")
end
end

defp base_email do
new_email()
|> from("[email protected]")
|> put_html_layout({CadetWeb.LayoutView, "email.html"})
end
end
3 changes: 3 additions & 0 deletions lib/cadet/jobs/scheduler.ex
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
# credo:disable-for-this-file Credo.Check.Readability.ModuleDoc
# @moduledoc is actually generated by a macro inside Quantum
defmodule Cadet.Jobs.Scheduler do
@moduledoc """
Quantum is used for scheduling jobs with cron jobs.
"""
use Quantum, otp_app: :cadet
end
6 changes: 6 additions & 0 deletions lib/cadet/mailer.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
defmodule Cadet.Mailer do
@moduledoc """
Mailer used to sent notification emails.
"""
use Bamboo.Mailer, otp_app: :cadet
end
Loading

0 comments on commit ed7f772

Please sign in to comment.