Skip to content

Commit

Permalink
Fix: apply suggestions
Browse files Browse the repository at this point in the history
  • Loading branch information
Rotheem committed Jan 2, 2025
1 parent 3b70db2 commit c7e5a3b
Show file tree
Hide file tree
Showing 5 changed files with 59 additions and 78 deletions.
1 change: 1 addition & 0 deletions app/core/groups/groups_type.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,4 +65,5 @@ def get_account_types_except_externals() -> list[AccountType]:
AccountType.staff,
AccountType.association,
AccountType.demo,
AccountType.other_school_student,
]
7 changes: 0 additions & 7 deletions app/core/models_core.py
Original file line number Diff line number Diff line change
Expand Up @@ -132,13 +132,6 @@ class CoreSchool(Base):
name: Mapped[str] = mapped_column(String, index=True, nullable=False, unique=True)
email_regex: Mapped[str] = mapped_column(String, nullable=False)

students: Mapped[list["CoreUser"]] = relationship(
"CoreUser",
back_populates="school",
lazy="selectin",
default_factory=list,
)


class CoreAssociationMembership(Base):
__tablename__ = "core_association_membership"
Expand Down
3 changes: 0 additions & 3 deletions app/core/schemas_core.py
Original file line number Diff line number Diff line change
Expand Up @@ -78,14 +78,11 @@ class CoreUserSimple(CoreUserBase):

id: str
account_type: AccountType
model_config = ConfigDict(from_attributes=True)


class CoreSchool(CoreSchoolSimple):
"""Schema for school's model similar to core_school table in database"""

model_config = ConfigDict(from_attributes=True)

students: list[CoreUserSimple] = []


Expand Down
40 changes: 13 additions & 27 deletions app/core/schools/endpoints_schools.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
)
async def read_schools(
db: AsyncSession = Depends(get_db),
user=Depends(is_user_an_ecl_member),
user: schemas_core.CoreUser = Depends(is_user_an_ecl_member),
):
"""
Return all schools from database as a list of dictionaries
Expand All @@ -50,7 +50,7 @@ async def read_schools(
async def read_school(
school_id: str,
db: AsyncSession = Depends(get_db),
user=Depends(is_user_in(GroupType.admin)),
user: schemas_core.CoreUser = Depends(is_user_in(GroupType.admin)),
):
"""
Return school with id from database as a dictionary. This includes a list of users being members of the school.
Expand All @@ -75,7 +75,7 @@ async def create_school(
user: schemas_core.CoreUser = Depends(is_user_in(GroupType.admin)),
):
"""
Create a new school.
Create a new school and add users to it based on the email regex.
**This endpoint is only usable by administrators**
"""
Expand Down Expand Up @@ -110,15 +110,9 @@ async def create_school(
),
)
await db.commit()
except ValueError as error:
await db.rollback()
raise HTTPException(status_code=422, detail=str(error))
except IntegrityError:
await db.rollback()
raise HTTPException(
status_code=400,
detail="School creation failed due to database integrity error",
)
raise
else:
return db_school

Expand All @@ -131,7 +125,7 @@ async def update_school(
school_id: str,
school_update: schemas_core.CoreSchoolUpdate,
db: AsyncSession = Depends(get_db),
user=Depends(is_user_in(GroupType.admin)),
user: schemas_core.CoreUser = Depends(is_user_in(GroupType.admin)),
):
"""
Update the name or the description of a school.
Expand Down Expand Up @@ -180,7 +174,11 @@ async def update_school(
account_type=AccountType.other_school_student,
),
)
await db.commit()
try:
await db.commit()
except IntegrityError:
await db.rollback()
raise


@router.delete(
Expand All @@ -190,7 +188,7 @@ async def update_school(
async def delete_school(
school_id: str,
db: AsyncSession = Depends(get_db),
user=Depends(is_user_in(GroupType.admin)),
user: schemas_core.CoreUser = Depends(is_user_in(GroupType.admin)),
):
"""
Delete school from database.
Expand All @@ -211,23 +209,11 @@ async def delete_school(
if school is None:
raise HTTPException(status_code=404, detail="School not found")

users = await cruds_users.get_users(db=db, schools_ids=[school_id])
for db_user in users:
await cruds_users.update_user(
db,
db_user.id,
schemas_core.CoreUserUpdateAdmin(
school_id=SchoolType.no_school.value,
account_type=AccountType.external,
),
)
await cruds_users.remove_users_from_school(db=db, school_id=school_id)

await cruds_schools.delete_school(db=db, school_id=school_id)
try:
await db.commit()
except IntegrityError:
await db.rollback()
raise HTTPException(
status_code=400,
detail="School deletion failed due to database integrity error",
)
raise
86 changes: 45 additions & 41 deletions app/core/users/cruds_users.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,51 +39,55 @@ async def get_users(
excluded_groups = excluded_groups or []
schools_ids = schools_ids or None

# We want, for each group that should be included check if
# - at least one of the user's groups match the expected group
included_group_condition = [
models_core.CoreUser.groups.any(
models_core.CoreGroup.id == group_id,
)
for group_id in included_groups
]
included_account_type_condition = or_(
False,
*[
models_core.CoreUser.account_type == account_type
for account_type in included_account_types
],
)
# We want, for each group that should not be included
# check that the following condition is false :
# - at least one of the user's groups match the expected group
excluded_group_condition = [
not_(
models_core.CoreUser.groups.any(
models_core.CoreGroup.id == group_id,
),
)
for group_id in excluded_groups
]
excluded_account_type_condition = [
not_(
models_core.CoreUser.account_type == account_type,
)
for account_type in excluded_account_types
]
school_condition = (
or_(
*[models_core.CoreUser.school_id == school_id for school_id in schools_ids],
)
if schools_ids
else and_(True)
)

result = await db.execute(
select(models_core.CoreUser).where(
and_(
True,
# We want, for each group that should be included check if
# - at least one of the user's groups match the expected group
*[
models_core.CoreUser.groups.any(
models_core.CoreGroup.id == group_id,
)
for group_id in included_groups
],
or_(
False,
*[
models_core.CoreUser.account_type == account_type
for account_type in included_account_types
],
),
*[
not_(
models_core.CoreUser.account_type == account_type,
)
for account_type in excluded_account_types
],
# We want, for each group that should not be included
# check that the following condition is false :
# - at least one of the user's groups match the expected group
*[
not_(
models_core.CoreUser.groups.any(
models_core.CoreGroup.id == group_id,
),
)
for group_id in excluded_groups
],
or_(
False,
*[
models_core.CoreUser.school_id == school_id
for school_id in schools_ids
],
)
if schools_ids
else and_(True),
*included_group_condition,
included_account_type_condition,
*excluded_account_type_condition,
*excluded_group_condition,
school_condition,
),
),
)
Expand Down

0 comments on commit c7e5a3b

Please sign in to comment.