Versioning and History

[geraldnyeo]

This PR implements the backend support for the "Versioning and History feature". It adds a "versions" table which tracks history for entries in the "answers" table, and implements the required version schema, controller, and routes.

This change is non-breaking and should be merged before source-academy/frontend#3659.

Database Changes

This PR adds a "versions" table with the following columns:

• id
• name
• version: the content of the answer is stored here
• answer_id
• restored
• restored_from
• inserted_at
• updated_at

Each entry in the table represents a version of an answer (from the answer table). There can be multiple versions for one answer.

The migration for this table has been added here: \priv\repo\migrations\20260219073155_create_versions.exs. To update the database to the latest version, run:

mix ecto.migrate

Database Design Decisions
Currently, each answer is indexed by a unique submission id + question id combination, which most methods and routes use. Multiple versions of the same answer would have the same submission id + question id combination, and storing them in the current answers table would introduce a lot of breaking changes, so we decided to store them in a separate versions table.

We also decided to store both the answer content in both the answer and versions table. Each version entry references an entry in the answers table (which cannot be null). Replacing the "answer" field in the answers table with some reference to the versions table (which also cannot be null) makes it difficult to create new answers, and requires an extra join every time an answer is searched for.

Routes

• GET /v2/courses/:course_id/assessments/question/:questionid/version/history
• POST /v2/courses/:course_id/assessments/question/:questionid/version/save
• PUT /v2/courses/:course_id/assessments/question/:questionid/version/:versionid/name

Additional Notes

• The schema.png in the README has not been updated to reflect these changes.

Related Issues / PRs

• Versioning and history for student submissions frontend#3074
• Versioning and history frontend#3659

[RichDom2185]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces backend support for versioning and history of answers, which is a significant feature. The implementation adds a new versions table, along with the necessary schema, controller, and routes. The changes are generally well-structured, but there are several critical issues related to error handling and control flow that could lead to crashes. Specifically, several functions don't handle potential {:error, ...} tuples from find_team, leading to MatchError exceptions. There are also opportunities to reduce code duplication and improve adherence to Elixir conventions. I've provided specific comments and suggestions to address these points.

[gemini-code-assist]

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

[gemini-code-assist]

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

[gemini-code-assist]

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

[gemini-code-assist]

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

[gemini-code-assist]

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)

[gemini-code-assist]

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.

[RichDom2185]

? What is this for?

[RichDom2185]

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