Skip to content
Draft
Show file tree
Hide file tree
Changes from 13 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
3 changes: 2 additions & 1 deletion lib/cadet/assessments/answer.ex
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ defmodule Cadet.Assessments.Answer do
alias Cadet.Accounts.CourseRegistration
alias Cadet.Assessments.Answer.AutogradingStatus
alias Cadet.Assessments.AnswerTypes.{MCQAnswer, ProgrammingAnswer, VotingAnswer}
alias Cadet.Assessments.{Question, QuestionType, Submission}
alias Cadet.Assessments.{Question, QuestionType, Submission, Version}
alias Cadet.AIComments.AIComment

@type t :: %__MODULE__{}
Expand All @@ -31,6 +31,7 @@ defmodule Cadet.Assessments.Answer do
belongs_to(:submission, Submission)
belongs_to(:question, Question)
has_many(:ai_comments, AIComment, on_delete: :delete_all)
has_many(:version, Version, on_delete: :delete_all)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

For clarity and to follow Elixir/Ecto conventions, has_many associations should use a plural name. The association to Version should be named :versions.

    has_many(:versions, Version, on_delete: :delete_all)


timestamps()
end
Expand Down
160 changes: 158 additions & 2 deletions lib/cadet/assessments/assessments.ex
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,16 @@ defmodule Cadet.Assessments do
CourseRegistrations
}

alias Cadet.Assessments.{Answer, Assessment, Query, Question, Submission, SubmissionVotes}
alias Cadet.Assessments.{
Answer,
Assessment,
Query,
Question,
Submission,
SubmissionVotes,
Version
}

alias Cadet.Autograder.GradingJob
alias Cadet.Courses.{Group, AssessmentConfig}
alias Cadet.Jobs.Log
Expand Down Expand Up @@ -1284,7 +1293,8 @@ defmodule Cadet.Assessments do
with {:ok, _team} <- find_team(question.assessment.id, cr_id),
{:ok, submission} <- find_or_create_submission(cr, question.assessment),
{:status, true} <- {:status, force_submit or submission.status != :submitted},
{:ok, _answer} <- insert_or_update_answer(submission, question, raw_answer, cr_id) do
{:ok, answer} <- insert_or_update_answer(submission, question, raw_answer, cr_id),
{:ok, _version} <- insert_version(question, answer, raw_answer) do
Logger.info("Successfully answered question #{question.id} for user #{cr_id}")
update_submission_status_router(submission, question)

Expand Down Expand Up @@ -3610,4 +3620,150 @@ defmodule Cadet.Assessments do

Repo.one(query)
end

def get_version(
question = %Question{},
cr = %CourseRegistration{}
) do
{:ok, team} = find_team(question.assessment.id, cr.id)

case team do
%Team{} ->
Version
|> join(:inner, [v], a in assoc(v, :answer))
|> join(:inner, [v, a], s in assoc(a, :submission))
|> where([v, a, s], a.question_id == ^question.id)
|> where([v, a, s], s.team_id == ^team.id)
|> Repo.all()

nil ->
Version
|> join(:inner, [v], a in assoc(v, :answer))
|> join(:inner, [v, a], s in assoc(a, :submission))
|> where([v, a, s], a.question_id == ^question.id)
|> where([v, a, s], s.student_id == ^cr.id)
|> Repo.all()
end
end
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

critical

This function has a few issues:

  1. Critical Bug: The line {:ok, team} = find_team(...) will raise a MatchError if find_team returns {:error, :team_not_found}, crashing the request. This needs to be handled, for example by using a with statement.
  2. Code Duplication: The query logic is duplicated inside the case statement. This can be refactored to build the query incrementally.
  3. Naming: The function is named get_version but it returns a list of versions (Repo.all()). It should be renamed to get_versions to better reflect what it does.
  4. Return Value: The function returns a raw list, which is inconsistent with other context functions that typically return {:ok, result} or {:error, reason}. This makes error handling in the controller difficult.

Here's a suggested refactoring that addresses these points. Note that you will also need to update the call site in CadetWeb.VersionsController.history/2.

  def get_versions(
        question = %Question{},
        cr = %CourseRegistration{}
      ) do
    with {:ok, team} <- find_team(question.assessment.id, cr.id) do
      base_query =
        Version
        |> join(:inner, [v], a in assoc(v, :answer))
        |> join(:inner, [v, a], s in assoc(a, :submission))
        |> where([v, a, s], a.question_id == ^question.id)

      query =
        case team do
          %Team{} ->
            where(base_query, [_v, _a, s], s.team_id == ^team.id)

          nil ->
            where(base_query, [_v, _a, s], s.student_id == ^cr.id)
        end

      {:ok, Repo.all(query)}
    else
      error -> error
    end
  end


def save_version(
question = %Question{},
cr = %CourseRegistration{id: cr_id},
raw_version
) do
if question.type == :voting do
{:error, {:bad_request, "Cannot save version for voting question"}}
end

with {:ok, _team} <- find_team(question.assessment.id, cr_id),
{:ok, submission} <- find_or_create_submission(cr, question.assessment),
{:ok, answer} <- find_or_create_answer(question, submission, raw_version),
{:ok, _version} <- insert_version(question, answer, raw_version) do
Logger.info("Successfully saved version for answer #{question.id} for user #{cr_id}")

# placeholder
{:ok, nil}
else
{:error, :team_not_found} ->
Logger.error("Team not found for question #{question.id} and user #{cr_id}")
{:error, {:bad_request, "Your existing Team has been deleted!"}}
end

{:ok, nil}
end
Comment on lines +3655 to +3678
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

critical

There are a couple of critical bugs in this function's control flow:

  1. The initial check if question.type == :voting returns an error tuple, but this doesn't halt the function's execution. The code continues to the with block.
  2. The expression {:ok, nil} at the end of the function will be returned if the else block of the with statement is executed, incorrectly masking the original error.

The function should be restructured to ensure correct control flow and error handling.

  def save_version(
        question = %Question{},
        cr = %CourseRegistration{id: cr_id},
        raw_version
      ) do
    if question.type == :voting do
      {:error, {:bad_request, "Cannot save version for voting question"}}
    else
      with {:ok, _team} <- find_team(question.assessment.id, cr_id),
           {:ok, submission} <- find_or_create_submission(cr, question.assessment),
           {:ok, answer} <- find_or_create_answer(question, submission, raw_version),
           {:ok, _version} <- insert_version(question, answer, raw_version) do
        Logger.info("Successfully saved version for answer #{question.id} for user #{cr_id}")

        # placeholder
        {:ok, nil}
      else
        {:error, :team_not_found} ->
          Logger.error("Team not found for question #{question.id} and user #{cr_id}")
          {:error, {:bad_request, "Your existing Team has been deleted!"}}

        error ->
          error
      end
    end
  end


defp find_or_create_answer(
question = %Question{},
submission = %Submission{},
raw_version
) do
case find_answer(question, submission) do
{:ok, answer} -> {:ok, answer}
{:error, _} -> create_new_answer(question, submission, raw_version)
end
end

defp find_answer(question = %Question{}, submission = %Submission{}) do
answer =
Answer
|> where(submission_id: ^submission.id)
|> where(question_id: ^question.id)
|> Repo.one()

if answer do
{:ok, answer}
else
{:error, nil}
end
end

defp create_new_answer(
question = %Question{},
submission = %Submission{},
raw_version
) do
answer_content = build_answer_content(raw_version, question.type)

%Answer{}
|> Answer.changeset(%{
answer: answer_content,
question_id: question.id,
submission_id: submission.id,
type: question.type
})
|> Repo.insert()
end

defp insert_version(
question = %Question{},
answer = %Answer{},
raw_version
) do
version_content = build_answer_content(raw_version, question.type)

%Version{}
|> Version.changeset(%{
version: version_content,
answer_id: answer.id
})
|> Repo.insert()
end

def name_version(
question = %Question{},
cr = %CourseRegistration{id: cr_id},
version_id,
name
) do
{:ok, team} = find_team(question.assessment.id, cr.id)

version =
case team do
%Team{} ->
Version
|> join(:inner, [v], a in assoc(v, :answer))
|> join(:inner, [v, a], s in assoc(a, :submission))
|> where([v, a, s], v.id == ^version_id)
|> where([v, a, s], s.team_id == ^team.id)
|> Repo.one()

nil ->
Version
|> join(:inner, [v], a in assoc(v, :answer))
|> join(:inner, [v, a], s in assoc(a, :submission))
|> where([v, a, s], v.id == ^version_id)
|> where([v, a, s], s.student_id == ^cr.id)
|> Repo.one()
end

case version do
nil ->
{:error, {:not_found, "Version not found"}}

version ->
version
|> Version.changeset(%{name: name})
|> Repo.update()
end
end
Comment on lines +3737 to +3780
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

critical

This function has similar issues to get_version:

  1. Critical Bug: The line {:ok, team} = find_team(...) will raise a MatchError if find_team returns an error, crashing the request.
  2. Code Duplication: The query logic is duplicated for team and non-team cases.

This should be refactored to handle the error and reduce duplication.

  def name_version(
        question = %Question{},
        cr = %CourseRegistration{id: cr_id},
        version_id,
        name
      ) do
    with {:ok, team} <- find_team(question.assessment.id, cr.id) do
      base_query =
        Version
        |> join(:inner, [v], a in assoc(v, :answer))
        |> join(:inner, [v, a], s in assoc(a, :submission))
        |> where([v, a, s], v.id == ^version_id)

      version =
        case team do
          %Team{} ->
            base_query
            |> where([_v, _a, s], s.team_id == ^team.id)
            |> Repo.one()

          nil ->
            base_query
            |> where([_v, _a, s], s.student_id == ^cr.id)
            |> Repo.one()
        end

      case version do
        nil ->
          {:error, {:not_found, "Version not found"}}

        version ->
          version
          |> Version.changeset(%{name: name})
          |> Repo.update()
      end
    else
      error -> error
    end
  end

end
24 changes: 24 additions & 0 deletions lib/cadet/assessments/version.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
defmodule Cadet.Assessments.Version do
use Ecto.Schema
import Ecto.Changeset

alias Cadet.Assessments.Answer

schema "versions" do
field(:version, :map)
field(:name, :string)
field(:restored, :boolean, default: false)
field(:restored_from, :id)

belongs_to(:answer, Answer)

timestamps()
end

@doc false
def changeset(version, attrs) do
version
|> cast(attrs, [:version, :name, :restored, :answer_id])
|> validate_required([:version, :restored, :answer_id])
end
end
77 changes: 77 additions & 0 deletions lib/cadet_web/controllers/versions_controller.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
defmodule CadetWeb.VersionsController do
@moduledoc """
Handles code versioning and history
"""
use CadetWeb, :controller
use PhoenixSwagger
require Logger

alias Cadet.Assessments

def history(conn, %{"questionid" => question_id}) do
course_reg = conn.assigns[:course_reg]

Logger.info(
"Fetching all versions for question #{question_id} for user #{course_reg.id} in course #{course_reg.course_id}"
)

with {:question, question} when not is_nil(question) <-
{:question, Assessments.get_question(question_id)},
{:versions, versions} <-
{:versions, Assessments.get_version(question, course_reg)} do
conn
|> put_status(:ok)
|> put_resp_content_type("application/json")
|> render("index.json", versions: versions)
else
{:question, nil} ->
conn
|> put_status(:not_found)
|> text("Question not found")
end
Comment on lines +11 to +42
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

This action doesn't handle potential errors from Assessments.get_version/2. As find_team/2 within get_version/2 can fail, it can crash the request.

After refactoring Assessments.get_version/2 to Assessments.get_versions/2 to return a tuple like {:ok, versions} or {:error, reason} (as suggested in another comment), you should update this with block to handle the error cases gracefully.

  def history(conn, %{"questionid" => question_id}) do
    course_reg = conn.assigns[:course_reg]

    Logger.info(
      "Fetching all versions for question #{question_id} for user #{course_reg.id} in course #{course_reg.course_id}"
    )

    with {:question, question} when not is_nil(question) <-
           {:question, Assessments.get_question(question_id)},
         {:ok, versions} <- Assessments.get_versions(question, course_reg) do
      conn
      |> put_status(:ok)
      |> put_resp_content_type("application/json")
      |> render("index.json", versions: versions)
    else
      {:question, nil} ->
        conn
        |> put_status(:not_found)
        |> text("Question not found")

      {:error, {:bad_request, message}} ->
        conn
        |> put_status(:bad_request)
        |> text(message)

      {:error, :team_not_found} ->
        conn
        |> put_status(:bad_request)
        |> text("Your existing Team has been deleted!")
    end
  end

end

def save(conn, %{"questionid" => question_id, "version" => version}) do
course_reg = conn.assigns[:course_reg]

with {:question, question} when not is_nil(question) <-
{:question, Assessments.get_question(question_id)},
{:ok, _nil} <- Assessments.save_version(question, course_reg, version) do
text(conn, "OK")
else
{:question, nil} ->
conn
|> put_status(:not_found)
|> text("Question not found")

{:error, {status, message}} ->
conn
|> put_status(status)
|> text(message)
end
end

def name(conn, %{
"questionid" => question_id,
"versionid" => version_id,
"name" => name
}) do
course_reg = conn.assigns[:course_reg]

with {:question, question} when not is_nil(question) <-
{:question, Assessments.get_question(question_id)},
{:ok, _nil} <- Assessments.name_version(question, course_reg, version_id, name) do
text(conn, "OK")
else
{:question, nil} ->
conn
|> put_status(:not_found)
|> text("Question not found")

{:error, {status, message}} ->
conn
|> put_status(status)
|> text(message)
end
end
end
4 changes: 4 additions & 0 deletions lib/cadet_web/router.ex
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,10 @@ defmodule CadetWeb.Router do
:check_last_modified
)

get("/assessments/question/:questionid/version/history", VersionsController, :history)
post("/assessments/question/:questionid/version/save", VersionsController, :save)
put("/assessments/question/:questionid/version/:versionid/name", VersionsController, :name)

get("/achievements", IncentivesController, :index_achievements)
get("/self/goals", IncentivesController, :index_goals)
post("/self/goals/:uuid/progress", IncentivesController, :update_progress)
Expand Down
20 changes: 20 additions & 0 deletions lib/cadet_web/views/versions_view.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
defmodule CadetWeb.VersionsView do
use CadetWeb, :view

def render("index.json", %{versions: versions}) do
render_many(versions, CadetWeb.VersionsView, "show.json", as: :version)
end

def render("show.json", %{version: version}) do
transform_map_for_view(version, %{
id: :id,
name: :name,
restored: :restored,
restored_from: :restored_from,
answer_id: :answer_id,
inserted_at: :inserted_at,
updated_at: :updated_at,
version: :version
})
end
end
7 changes: 7 additions & 0 deletions lib/mix/tasks/token.ex
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,13 @@ defmodule Mix.Tasks.Cadet.Token do
course_id: course.id,
role: role
})

%User{}
|> User.changeset(%{
name: "Test#{role_capitalized}",
username: "test_#{role}",
provider: "test"
})
Comment on lines +111 to +117
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

? What is this for?

|> Repo.insert!()

new_user
Expand Down
49 changes: 49 additions & 0 deletions priv/repo/migrations/20260219073155_create_versions.exs
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
defmodule Cadet.Repo.Migrations.CreateVersions do
use Ecto.Migration
import Ecto.Query, only: [from: 2]

def change do
create table(:versions) do
add(:version, :map)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not a big fan of the column name, can it be something like answer instead? version suggests a version number

add(:name, :string)
add(:restored, :boolean, default: false, null: false)
add(:answer_id, references(:answers, on_delete: :delete_all))
add(:restored_from, references(:versions, on_delete: :nothing))

timestamps()
end

create(index(:versions, [:answer_id]))
create(index(:versions, [:restored_from]))

# Backfill data from answers table
execute(fn ->
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Using execute/1 with an anonymous function inside a change/0 function makes this migration irreversible. Ecto cannot automatically determine how to reverse the data backfill operation. For migrations that are not automatically reversible, it's best practice to implement up/0 and down/0 functions explicitly. The down/0 function would typically just drop the table.

answers =
from(a in "answers",
select: %{
id: a.id,
answer: a.answer,
inserted_at: a.inserted_at,
updated_at: a.updated_at
},
join: q in "questions",
on: q.id == a.question_id,
where: q.type != "voting"
)
|> repo().all()

versions =
answers
|> Enum.map(fn a ->
%{
answer_id: a.id,
version: a.answer,
inserted_at: a.inserted_at,
updated_at: a.updated_at
}
end)

repo().insert_all("versions", versions)
end)
end
end
Loading
Loading