Skip to content

Commit

Permalink
Revert "Upsert LMSUser everytime we upsert User"
Browse files Browse the repository at this point in the history
This reverts commit f524354.
  • Loading branch information
marcospri committed Aug 23, 2024
1 parent 22d5c34 commit 92ce837
Show file tree
Hide file tree
Showing 2 changed files with 1 addition and 55 deletions.
38 changes: 0 additions & 38 deletions lms/services/user.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,13 @@
Assignment,
AssignmentMembership,
GroupingMembership,
LMSUser,
LMSUserApplicationInstance,
LTIRole,
LTIUser,
RoleScope,
RoleType,
User,
)
from lms.services.course import CourseService
from lms.services.upsert import bulk_upsert


class UserNotFound(Exception):
Expand Down Expand Up @@ -67,43 +64,8 @@ def upsert_user(self, lti_user: LTIUser) -> User:
# We are only storing emails for teachers now.
user.email = lti_user.email

self._upsert_lms_user(user)

return user

def _upsert_lms_user(self, user: User) -> LMSUser:
"""Upsert LMSUser based on a User object."""
self._db.flush() # Make sure User has hit the DB on the current transaction

lms_user = bulk_upsert(
self._db,
LMSUser,
[
{
"tool_consumer_instance_guid": user.application_instance.tool_consumer_instance_guid,
"lti_user_id": user.user_id,
"h_userid": user.h_userid,
"email": user.email,
"display_name": user.display_name,
}
],
index_elements=["h_userid"],
update_columns=["updated", "display_name", "email"],
).one()
bulk_upsert(
self._db,
LMSUserApplicationInstance,
[
{
"application_instance_id": user.application_instance_id,
"lms_user_id": lms_user.id,
}
],
index_elements=["application_instance_id", "lms_user_id"],
update_columns=["updated"],
)
return lms_user

@lru_cache(maxsize=128)
def get(self, application_instance, user_id: str) -> User:
"""
Expand Down
18 changes: 1 addition & 17 deletions tests/unit/lms/services/user_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,8 @@

import pytest
from h_matchers import Any
from sqlalchemy import select

from lms.models import LMSUser, RoleScope, RoleType, User
from lms.models import RoleScope, RoleType, User
from lms.services import UserService
from lms.services.user import UserNotFound, factory
from tests import factories
Expand All @@ -31,7 +30,6 @@ def test_upsert_user(self, service, lti_user, db_session):
}
)
assert saved_user == user
self.assert_lms_user(db_session, user)

@pytest.mark.usefixtures("user_is_learner")
def test_upsert_user_doesnt_save_email_for_students(
Expand All @@ -42,7 +40,6 @@ def test_upsert_user_doesnt_save_email_for_students(
saved_user = db_session.query(User).order_by(User.id.desc()).first()
assert saved_user.roles == lti_user.roles
assert not saved_user.email
self.assert_lms_user(db_session, saved_user)

@pytest.mark.usefixtures("user")
def test_upsert_user_with_an_existing_user(self, service, lti_user, db_session):
Expand All @@ -52,7 +49,6 @@ def test_upsert_user_with_an_existing_user(self, service, lti_user, db_session):
assert saved_user.id == user.id
assert saved_user.roles == lti_user.roles
assert user == saved_user
self.assert_lms_user(db_session, user)

@pytest.mark.usefixtures("user")
def test_upsert_user_doesnt_save_email_for_existing_students(
Expand All @@ -65,7 +61,6 @@ def test_upsert_user_doesnt_save_email_for_existing_students(
saved_user = db_session.query(User).order_by(User.id.desc()).first()
assert saved_user.roles == lti_user.roles
assert not saved_user.email
self.assert_lms_user(db_session, saved_user)

def test_get(self, user, service):
db_user = service.get(user.application_instance, user.user_id)
Expand Down Expand Up @@ -174,17 +169,6 @@ def test_get_users_by_instructor_h_userid(

assert db_session.scalars(query).all() == [student_in_assigment]

def assert_lms_user(self, db_session, user):
"""Assert the corresponding LMSUser to user exists in the DB with the same attributes."""

lms_user = db_session.scalars(
select(LMSUser).where(LMSUser.h_userid == user.h_userid)
).one()

assert lms_user.display_name == user.display_name
assert lms_user.email == user.email
assert lms_user.updated == user.updated

@pytest.fixture
def course(self, application_instance, db_session):
course = factories.Course(application_instance=application_instance)
Expand Down

0 comments on commit 92ce837

Please sign in to comment.