[BUG FIX] [MER-4138] Fix launch issues (#5332)

* db transaction, add lti_institution_id

* bump version

* bump version

* update unit test

* Auto format

---------

Co-authored-by: darrensiegel <[email protected]>

lib/oli/accounts.ex

@@ -350,24 +350,90 @@ defmodule Oli.Accounts do
   """
 
   def insert_or_update_lms_user(%{sub: sub} = changes, institution_id) do
+    # First see if we can find a user that matches the sub and institution id exactly. This
+    # will end up being the most common case (on all launches for a user beyond the first)
+    user =
+      case Repo.get_by(User, sub: sub, lti_institution_id: institution_id) do
+        # If not found directly, do another read of ALL users with this sub.  This step
+        # isn't strictly necessary (we could just call find_user_through_enrollment), but
+        # do it to make this more robust by reducing the need to rely on the enrollment.
+        nil ->
+          case get_all_users_by_sub(sub) do
+            # If no users with this sub, we can be absolutely sure that we need to create a new user
+            [] -> nil
+            # Otherwise, we now need to check to see if one of these users is enrolled in a section
+            # that is pinned to this institution
+            _ -> find_user_through_enrollment(sub, institution_id)
+          end
+
+        user ->
+          user
+      end
+
+    case user do
+      nil -> create_lms_user(changes)
+      user -> update_lms_user(user, changes)
+    end
+  end
+
+  defp get_all_users_by_sub(sub) do
+    from(u in User, where: u.sub == ^sub) |> Repo.all()
+  end
+
+  defp find_user_through_enrollment(sub, institution_id) do
     # using enrollment records, we can infer the user's institution. This is because
     # an LTI user can be enrolled in multiple sections, but all sections must be from
     # the same institution.
-    from(e in Enrollment,
-      join: s in Section,
-      on: e.section_id == s.id,
-      join: u in User,
-      on: e.user_id == u.id,
-      join: institution in Institution,
-      on: s.institution_id == institution.id,
-      where:
-        u.sub == ^sub and institution.id == ^institution_id and s.status == :active and
-          e.status == :enrolled,
-      limit: 1,
-      select: u
-    )
-    |> Repo.one()
-    |> insert_or_update_external_user(changes)
+    results =
+      from(e in Enrollment,
+        join: s in Section,
+        on: e.section_id == s.id,
+        join: u in User,
+        on: e.user_id == u.id,
+        join: institution in Institution,
+        on: s.institution_id == institution.id,
+        where: u.sub == ^sub and institution.id == ^institution_id,
+        select: u,
+        order_by: [desc: u.inserted_at]
+      )
+      |> Repo.all()
+
+    # We must handle the fact that duplicate records can exist in the result set, in
+    # this case we select the "most recently inserted" user record
+    case results do
+      [user | _] -> user
+      [] -> nil
+    end
+  end
+
+  defp create_lms_user(%{sub: sub} = changes) do
+    %User{sub: sub, independent_learner: false}
+    |> User.noauth_changeset(changes)
+    |> Repo.insert()
+    |> case do
+      {:ok, %User{id: user_id}} = res ->
+        AccountLookupCache.delete("user_#{user_id}")
+
+        res
+
+      error ->
+        error
+    end
+  end
+
+  defp update_lms_user(%User{} = user, changes) do
+    user
+    |> User.noauth_changeset(changes)
+    |> Repo.update()
+    |> case do
+      {:ok, %User{id: user_id}} = res ->
+        AccountLookupCache.delete("user_#{user_id}")
+
+        res
+
+      error ->
+        error
+    end
   end
 
   @doc """

lib/oli/accounts/schemas/user.ex

@@ -49,8 +49,9 @@ defmodule Oli.Accounts.User do
 
     pow_user_fields()
 
-    # A user may optionally be linked to an author account
+    # A user may optionally be linked to an author account and Institution
     belongs_to :author, Oli.Accounts.Author
+    field :lti_institution_id, :integer, default: nil
 
     has_many :enrollments, Oli.Delivery.Sections.Enrollment, on_delete: :delete_all
 
@@ -112,6 +113,7 @@ defmodule Oli.Accounts.User do
       :phone_number_verified,
       :address,
       :author_id,
+      :lti_institution_id,
       :guest,
       :independent_learner,
       :research_opt_out,
@@ -156,6 +158,7 @@ defmodule Oli.Accounts.User do
       :phone_number_verified,
       :address,
       :author_id,
+      :lti_institution_id,
       :guest,
       :independent_learner,
       :research_opt_out,
@@ -237,6 +240,7 @@ defmodule Oli.Accounts.User do
       :phone_number_verified,
       :address,
       :author_id,
+      :lti_institution_id,
       :guest,
       :independent_learner,
       :research_opt_out,
@@ -321,7 +325,7 @@ defimpl Lti_1p3.Tool.Lti_1p3_User, for: Oli.Accounts.User do
         preload: [:context_roles],
         join: s in Section,
         on: e.section_id == s.id,
-        where: e.user_id == ^user_id and s.slug == ^section_slug and s.status == :active,
+        where: e.user_id == ^user_id and s.slug == ^section_slug,
         select: e
 
     case Repo.one(query) do

lib/oli_web/controllers/lti_controller.ex

@@ -48,19 +48,27 @@ defmodule OliWeb.LtiController do
 
     case Lti_1p3.Tool.LaunchValidation.validate(params, session_state) do
       {:ok, lti_params} ->
-        # cache user lti params and store the id in the current session
-        case LtiParams.create_or_update_lti_params(lti_params) do
-          {:ok, %{id: lti_params_id}} ->
-            conn = LtiSession.put_session_lti_params(conn, lti_params_id)
+        result =
+          Oli.Repo.transaction(fn ->
+            # cache user lti params and store the id in the current session
+            case LtiParams.create_or_update_lti_params(lti_params) do
+              {:ok, %{id: lti_params_id}} ->
+                conn = LtiSession.put_session_lti_params(conn, lti_params_id)
 
-            # handle the valid lti launch
-            handle_valid_lti_1p3_launch(conn, lti_params)
+                # handle the valid lti launch
+                handle_valid_lti_1p3_launch(conn, lti_params)
 
-          _ ->
-            {_error_id, error_msg} =
-              log_error("An error occurred while creating/updating LTI params")
+              _ ->
+                {_error_id, error_msg} =
+                  log_error("An error occurred while creating/updating LTI params")
 
-            throw(error_msg)
+                throw(error_msg)
+            end
+          end)
+
+        case result do
+          {:ok, conn} -> conn
+          e -> e
         end
 
       {:error, %{reason: :invalid_registration, msg: _msg, issuer: issuer, client_id: client_id}} ->
@@ -383,7 +391,8 @@ defmodule OliWeb.LtiController do
                  locale: lti_params["locale"],
                  phone_number: lti_params["phone_number"],
                  phone_number_verified: lti_params["phone_number_verified"],
-                 address: lti_params["address"]
+                 address: lti_params["address"],
+                 lti_institution_id: institution.id
                },
                institution.id
              ) do

priv/repo/migrations/20250104141507_add_institution_to_user.exs

@@ -0,0 +1,11 @@
+defmodule Oli.Repo.Migrations.AddInstitutionToUser do
+  use Ecto.Migration
+
+  def change do
+    alter table(:users) do
+      add :lti_institution_id, :integer, default: nil
+    end
+
+    create unique_index(:users, [:sub, :lti_institution_id])
+  end
+end

test/oli/lti_accounts_test.exs

@@ -0,0 +1,74 @@
+defmodule Oli.AccountsTest do
+  use Oli.DataCase
+
+  alias Oli.Accounts
+  alias Oli.Accounts.User
+  alias Lti_1p3.Tool.ContextRoles
+
+  def make_user(institution_id) do
+    %{
+      sub: "1234-1234",
+      name: "User Name",
+      given_name: "User",
+      family_name: "Name",
+      middle_name: "",
+      password: "password",
+      password_confirmation: "password",
+      email: "[email protected]",
+      email_verified: true,
+      lti_institution_id: institution_id
+    }
+  end
+
+  describe "insert or update" do
+    setup do
+      Seeder.base_project_with_resource2()
+      |> Seeder.create_section()
+    end
+
+    test "no user at all exists", %{section: section} do
+      institution_id = section.institution_id
+
+      {:ok, user1} = Accounts.insert_or_update_lms_user(make_user(institution_id), institution_id)
+
+      assert user1.lti_institution_id == institution_id
+
+      # Read it again to make sure it was inserted correctly
+      user = Oli.Repo.get_by(User, sub: user1.sub, lti_institution_id: institution_id)
+      assert user.lti_institution_id == institution_id
+      assert user.id == user1.id
+
+      # Now try to insert it again, but this time it will be an update
+      {:ok, user2} = Accounts.insert_or_update_lms_user(make_user(institution_id), institution_id)
+      assert user1.id == user2.id
+    end
+
+    test "a user exists already with this sub, but with nil institution", %{section: section} do
+      institution_id = section.institution_id
+
+      # Insert a user with nil institution
+      {:ok, user} = Oli.Repo.insert(User.changeset(%User{}, make_user(nil)))
+
+      # Now a call to insert_or_update_lms_user should create a new user with the institution_id
+      # because there is no enrollment record
+      {:ok, user1} = Accounts.insert_or_update_lms_user(make_user(institution_id), institution_id)
+
+      refute user1.id == user.id
+    end
+
+    test "a user exists already with this sub, but with nil institution AND enrollment", %{
+      section: section
+    } do
+      institution_id = section.institution_id
+
+      # Insert a user with nil institution and enroll it in a section
+      {:ok, user} = Oli.Repo.insert(User.changeset(%User{}, make_user(nil)))
+      Oli.Delivery.Sections.enroll(user.id, section.id, [ContextRoles.get_role(:context_learner)])
+
+      # Now a call to insert_or_update_lms_user should UPDATE that user, not create a new one
+      {:ok, user1} = Accounts.insert_or_update_lms_user(make_user(institution_id), institution_id)
+
+      assert user1.id == user.id
+    end
+  end
+end