Skip to content
Open
Show file tree
Hide file tree
Changes from 2 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
2 changes: 2 additions & 0 deletions Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ WORKDIR /usr/src/fastapi
# Copy the source code
COPY ./src .

COPY ./alembic.ini /usr/src/fastapi/alembic.ini

Choose a reason for hiding this comment

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

This line duplicates an earlier COPY ./alembic.ini (you previously copied it to /usr/src/alembic/alembic.ini). If you need alembic.ini in both locations keep it, otherwise remove this redundant copy to avoid confusion.

Choose a reason for hiding this comment

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

This COPY command is redundant. alembic.ini is already copied to /usr/src/alembic/alembic.ini earlier (line 25), which is where the ALEMBIC_CONFIG environment variable points (line 7). Removing this line would simplify the Dockerfile without impacting functionality.


Choose a reason for hiding this comment

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

General note: the Dockerfile currently installs only main dependencies (poetry install --no-root --only main), which will omit dev dependencies like uvicorn/watchfiles needed for the development server. For the development image you should install dev dependencies (for example by running poetry install --no-root --with dev or not using --only main) so the /commands/run_web_server_dev.sh script can run. Also consider adding a default CMD/ENTRYPOINT (or document that docker-compose supplies it).

# Copy commands
COPY ./commands /commands

Expand Down
9 changes: 7 additions & 2 deletions docker-compose-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,15 @@ services:
context: .
dockerfile: ./docker/tests/Dockerfile
container_name: backend_theater_test
command: [ "pytest", "-c", "/usr/src/config/pytest.ini",
"-m", "e2e", "--maxfail=5", "--disable-warnings", "-v", "--tb=short"]
command: >

Choose a reason for hiding this comment

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

The command is executed via sh -c " ... ". This is fine, but confirm the working directory and mounted files inside the test image so both Alembic and pytest find their configs and the application code as expected.

sh -c "
alembic upgrade head &&

Choose a reason for hiding this comment

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

You run alembic upgrade head before running pytest. This is correct in principle, but because ALEMBIC_CONFIG path is incorrect (see comment on line 15) this step will likely fail. Also ensure the database is reachable/ready before running Alembic (consider a short wait or a retry loop).

pytest -c /usr/src/config/pytest.ini -m e2e --maxfail=5 --disable-warnings -v --tb=short

Choose a reason for hiding this comment

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

The pytest command uses -c /usr/src/config/pytest.ini. Ensure pytest.ini actually exists at that path inside the container (either include it in the image or mount it). Otherwise pytest will run with default settings or fail to load the expected configuration.

"
environment:
- PYTHONPATH=/usr/src/fastapi
- ENVIRONMENT=testing
- ALEMBIC_CONFIG=/usr/src/config/alembic.ini
- EMAIL_HOST=mailhog_theater_test
- EMAIL_PORT=1025
- EMAIL_HOST_USER=testuser@mate.com
Expand All @@ -27,6 +31,7 @@ services:
condition: service_healthy
volumes:
- ./src:/usr/src/fastapi
- ./alembic.ini:/usr/src/config/alembic.ini

Choose a reason for hiding this comment

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

You mount ./alembic.ini to /usr/src/config/alembic.ini here — that does not match the ALEMBIC_CONFIG env value on line 15. Align the mounted destination and ALEMBIC_CONFIG value so Alembic can locate the config file.

networks:
- theater_network_test

Expand Down
19 changes: 17 additions & 2 deletions docker/tests/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,16 @@ FROM python:3.10-slim
ENV PYTHONDONTWRITEBYTECODE=1
ENV PYTHONUNBUFFERED=1
ENV PIP_NO_CACHE_DIR=off
ENV ALEMBIC_CONFIG=/usr/src/config/alembic.ini

# Installing dependencies
RUN apt update
RUN apt update && apt install -y \
gcc \
libpq-dev \
netcat-openbsd \
postgresql-client \
dos2unix \
&& apt clean

# Install Poetry
RUN python -m pip install --upgrade pip && \
Expand All @@ -27,10 +34,18 @@ WORKDIR /usr/src/poetry

# Install dependencies with Poetry
RUN poetry lock
RUN poetry install --no-root --only main
RUN poetry install --no-root

# Selecting a working directory
WORKDIR /usr/src/fastapi

# Copy the source code
COPY ./src .

COPY ./alembic.ini /usr/src/config/alembic.ini

COPY ./commands /commands
RUN dos2unix /commands/*.sh && chmod +x /commands/*.sh

CMD ["pytest", "-c", "/usr/src/config/pytest.ini", "-m", "e2e", "--maxfail=5", "--disable-warnings", "-v", "--tb=short"]

19 changes: 19 additions & 0 deletions src/config/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,30 @@ class Settings(BaseAppSettings):
JWT_SIGNING_ALGORITHM: str = os.getenv("JWT_SIGNING_ALGORITHM", "HS256")


# class TestingSettings(BaseAppSettings):
# SECRET_KEY_ACCESS: str = "SECRET_KEY_ACCESS"
# SECRET_KEY_REFRESH: str = "SECRET_KEY_REFRESH"
# JWT_SIGNING_ALGORITHM: str = "HS256"
#
# def model_post_init(self, __context: dict[str, Any] | None = None) -> None:
# object.__setattr__(self, 'PATH_TO_DB', ":memory:")
# object.__setattr__(
# self,
# 'PATH_TO_MOVIES_CSV',
# str(self.BASE_DIR / "database" / "seed_data" / "test_data.csv")
# )

Choose a reason for hiding this comment

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

There is a commented-out alternate TestingSettings block — remove or clearly document why it’s kept. Leaving duplicate implementations commented in the code is confusing and error-prone.


class TestingSettings(BaseAppSettings):
SECRET_KEY_ACCESS: str = "SECRET_KEY_ACCESS"
SECRET_KEY_REFRESH: str = "SECRET_KEY_REFRESH"
JWT_SIGNING_ALGORITHM: str = "HS256"

POSTGRES_USER: str = "admin"
POSTGRES_PASSWORD: str = "some_password"
POSTGRES_HOST: str = "postgres_theater"
POSTGRES_DB_PORT: int = 5432
POSTGRES_DB: str = "movies_db"

Choose a reason for hiding this comment

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

TestingSettings still defines Postgres connection values (POSTGRES_USER, POSTGRES_PASSWORD, POSTGRES_HOST, POSTGRES_DB_PORT, POSTGRES_DB). Tests should use SQLite (and model_post_init correctly sets PATH_TO_DB to ':memory:'). Consider removing or documenting these Postgres fields in TestingSettings so they don't mislead readers about the test DB being SQLite.

Choose a reason for hiding this comment

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

The TestingSettings class is currently configured for PostgreSQL. The task explicitly requires a lightweight SQLite database for testing, particularly for E2E tests. Please remove these PostgreSQL-specific environment variables and ensure TestingSettings aligns with the SQLite requirement.


def model_post_init(self, __context: dict[str, Any] | None = None) -> None:
object.__setattr__(self, 'PATH_TO_DB', ":memory:")
object.__setattr__(
Expand Down
33 changes: 32 additions & 1 deletion src/routes/accounts.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
from datetime import datetime, timezone
from typing import cast

from fastapi import APIRouter, Depends, status, HTTPException
from fastapi import APIRouter, Depends, status, HTTPException, BackgroundTasks
from sqlalchemy import select, delete
from sqlalchemy.exc import SQLAlchemyError
from sqlalchemy.ext.asyncio import AsyncSession
Expand Down Expand Up @@ -33,6 +33,9 @@
)
from security.interfaces import JWTAuthManagerInterface

from database.session_postgresql import get_postgresql_db

Choose a reason for hiding this comment

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

The module imports get_postgresql_db and the routes use it as the DB dependency. The task requirements mandate routes should accept db: AsyncSession = Depends(get_db) (so DI and session lifecycle behave as required). Replace this import/usage with get_db to satisfy the checklist.

from notifications.emails import EmailSender

Choose a reason for hiding this comment

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

EmailSender is imported here but never used. Per the requirements emails must be sent using the injected get_accounts_email_notificator (EmailSenderInterface). Remove this unused import to avoid confusion.

Choose a reason for hiding this comment

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

These imports (get_postgresql_db and EmailSender) appear to be unused. Since you are using dependency injection for the email sender and get_db for the database session, these can likely be removed to keep your imports clean.


router = APIRouter()


Expand Down Expand Up @@ -67,7 +70,9 @@
)
async def register_user(
user_data: UserRegistrationRequestSchema,
background_tasks: BackgroundTasks,
db: AsyncSession = Depends(get_db),
email_sender: EmailSenderInterface = Depends(get_accounts_email_notificator)
) -> UserRegistrationResponseSchema:
"""
Endpoint for user registration.
Expand Down Expand Up @@ -120,6 +125,13 @@ async def register_user(

await db.commit()
await db.refresh(new_user)
activation_link = f"http://127.0.0.1/accounts/activate/?token={activation_token.token}&email={user_data.email}"

background_tasks.add_task(
email_sender.send_activation_email,
new_user.email,
activation_link,
)
except SQLAlchemyError as e:
await db.rollback()
raise HTTPException(
Expand Down Expand Up @@ -233,7 +245,9 @@ async def activate_account(
)
async def request_password_reset_token(
data: PasswordResetRequestSchema,
background_tasks: BackgroundTasks,
db: AsyncSession = Depends(get_db),
email_sender: EmailSenderInterface = Depends(get_accounts_email_notificator),
) -> MessageResponseSchema:
"""
Endpoint to request a password reset token.
Expand Down Expand Up @@ -262,6 +276,12 @@ async def request_password_reset_token(
reset_token = PasswordResetTokenModel(user_id=cast(int, user.id))
db.add(reset_token)
await db.commit()
reset_link = f"http://127.0.0.1/accounts/reset-password/complete/?token={reset_token.token}&email={user.email}"

Choose a reason for hiding this comment

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

request_password_reset_token uses BackgroundTasks and the injected email_sender correctly — keep this pattern. However note earlier comment: replace get_postgresql_db with get_db for the db dependency to meet the DI requirement.

background_tasks.add_task(
email_sender.send_password_reset_email,
user.email,
reset_link
)

return MessageResponseSchema(
message="If you are registered, you will receive an email with instructions."
Expand Down Expand Up @@ -313,7 +333,10 @@ async def request_password_reset_token(
)
async def reset_password(
data: PasswordResetCompleteRequestSchema,
background_tasks: BackgroundTasks,

Choose a reason for hiding this comment

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

The reset_password endpoint properly schedules the password-reset-complete email via BackgroundTasks and email_sender. Ensure consistency by also switching the db dependency here to get_db (instead of get_postgresql_db).

db: AsyncSession = Depends(get_db),
email_sender: EmailSenderInterface = Depends(get_accounts_email_notificator),

) -> MessageResponseSchema:
"""
Endpoint for resetting a user's password.
Expand Down Expand Up @@ -360,6 +383,7 @@ async def reset_password(
if expires_at < datetime.now(timezone.utc):
await db.run_sync(lambda s: s.delete(token_record))
await db.commit()

raise HTTPException(
status_code=status.HTTP_400_BAD_REQUEST,
detail="Invalid email or token."
Expand All @@ -369,6 +393,13 @@ async def reset_password(
user.password = data.password
await db.run_sync(lambda s: s.delete(token_record))
await db.commit()

login_link = "http://127.0.0.1/accounts/login/"
background_tasks.add_task(
email_sender.send_password_reset_complete_email,
user.email,
login_link
)
except SQLAlchemyError:
await db.rollback()
raise HTTPException(
Expand Down
172 changes: 170 additions & 2 deletions src/routes/profiles.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,173 @@
from fastapi import APIRouter
from fastapi import APIRouter, Depends, HTTPException, status, UploadFile, BackgroundTasks
from sqlalchemy.ext.asyncio import AsyncSession
from sqlalchemy import select
from config.settings import Settings

Choose a reason for hiding this comment

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

The Settings import is unused in this file. Please remove it to keep the code clean and prevent unnecessary imports.


from database import get_db, ActivationTokenModel, UserGroupEnum
from database.models.accounts import UserModel, UserProfileModel
from schemas.profiles import ProfileCreateSchema, ProfileResponseSchema
from schemas.accounts import UserActivationRequestSchema, MessageResponseSchema
from security.token_manager import JWTAuthManager
from storages.interfaces import S3StorageInterface
from config.dependencies import get_s3_storage_client, get_accounts_email_notificator
from validation.profile import validate_image
from notifications.interfaces import EmailSenderInterface
from fastapi.security import OAuth2PasswordBearer
import os

router = APIRouter()

# Write your code here
oauth2_scheme = OAuth2PasswordBearer(tokenUrl="/api/v1/accounts/login/")

token_manager = JWTAuthManager(
secret_key_access=os.getenv("SECRET_KEY_ACCESS", "default_access_secret"),
secret_key_refresh=os.getenv("SECRET_KEY_REFRESH", "default_refresh_secret"),
algorithm=os.getenv("JWT_SIGNING_ALGORITHM", "HS256"),
)

Choose a reason for hiding this comment

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

Instantiating JWTAuthManager directly in the route file bypasses the project's dependency injection system and uses potentially insecure default secrets. For consistency, testability, and security, you should inject this using the provided dependency function, get_jwt_auth_manager, within your get_current_user function.


async def get_current_user(
token: str = Depends(oauth2_scheme),
db: AsyncSession = Depends(get_db),
) -> UserModel:
try:
payload = token_manager.decode_access_token(token)
user_id = payload.get("sub")
if not user_id:
raise HTTPException(
status_code=status.HTTP_401_UNAUTHORIZED,
detail="Invalid token: missing subject"
)

stmt = select(UserModel).where(UserModel.id == user_id)
result = await db.execute(stmt)
user = result.scalars().first()

if not user:
raise HTTPException(
status_code=status.HTTP_404_NOT_FOUND,
detail="User not found"
)

return user
except Exception:
raise HTTPException(
status_code=status.HTTP_401_UNAUTHORIZED,
detail="Invalid or expired token"

Choose a reason for hiding this comment

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

The get_current_user function catches a generic Exception. The task requires specific error messages for 'Token has expired.' (HTTP 401). Catching BaseSecurityError (or more specific exceptions from token_manager.py) would allow for more precise error handling and messaging, matching the requirement.

)

@router.post("/profiles/", response_model=ProfileResponseSchema)
async def create_profile(
profile: ProfileCreateSchema,
db: AsyncSession = Depends(get_db),
current_user: UserModel = Depends(get_current_user),
):
stmt = select(UserProfileModel).where(UserProfileModel.user_id == current_user.id)
result = await db.execute(stmt)
existing_profile = result.scalars().first()
if existing_profile:
raise HTTPException(
status_code=status.HTTP_400_BAD_REQUEST,
detail="Profile already exists."
)

new_profile = UserProfileModel(
user_id=current_user.id,
first_name=profile.first_name,
last_name=profile.last_name,
gender=profile.gender,
date_of_birth=profile.date_of_birth,
info=profile.info,
)
db.add(new_profile)
await db.commit()
await db.refresh(new_profile)

return new_profile

Choose a reason for hiding this comment

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

The /profiles/ endpoint duplicates functionality with /users/{user_id}/profile/ and is not explicitly required by the task. The task focuses on the /users/{user_id}/profile/ endpoint for user profile creation. Please remove this duplicated endpoint to streamline the API and avoid redundancy.




@router.post("/users/{user_id}/profile/",
response_model=ProfileResponseSchema,
status_code=status.HTTP_201_CREATED
)
async def create_user_profile(
user_id: int,
profile_data: ProfileCreateSchema = Depends(),
avatar: UploadFile | None = None,
db: AsyncSession = Depends(get_db),
current_user: UserModel = Depends(get_current_user),
s3_client: S3StorageInterface = Depends(get_s3_storage_client),
):
if current_user.id != user_id:
raise HTTPException(
status_code=status.HTTP_403_FORBIDDEN,
detail="Not authenticated to create profile for another user."

Choose a reason for hiding this comment

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

The task specifies that if a regular user tries to create a profile for another user, the error message should be: "You don't have permission to edit this profile.". Your current message "Not authenticated to create profile for another user." is slightly different. Please ensure the message matches the requirement exactly.

)

Choose a reason for hiding this comment

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

The task requires checking if the user_id from the path exists and is active. While current_user confirms the authenticated user is active, this check doesn't verify the status of the user_id from the path if it happens to be the same as current_user.id but the user's is_active status somehow changed or wasn't verified here. An explicit check if not user.is_active: (where user is fetched by user_id) should be added here to fulfill the requirement "User not found or not active." more precisely.


stmt = select(UserProfileModel).where(UserProfileModel.user_id == user_id)
result = await db.execute(stmt)
existing_profile = result.scalars().first()
if existing_profile:
raise HTTPException(
status_code=status.HTTP_400_BAD_REQUEST,
detail="Profile already exists."
)

avatar_url = None
if avatar:
validate_image(avatar)
avatar_key = f"avatars/{user_id}_avatar.jpg"
await s3_client.upload_fileobj(avatar.file, avatar_key)
avatar_url = await s3_client.get_file_url(avatar_key)

Choose a reason for hiding this comment

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

The task explicitly states: If the avatar upload fails, the endpoint must return: {"detail": "Failed to upload avatar. Please try again later."} with Error Code: 500 Internal Server Error. The current implementation lacks a try-except block around the s3_client operations to handle potential upload failures and return the specified error message.


new_profile = UserProfileModel(
user_id=user_id,
first_name=profile_data.first_name,
last_name=profile_data.last_name,
gender=profile_data.gender,
date_of_birth=profile_data.date_of_birth,
info=profile_data.info,
avatar=avatar_url,
)
db.add(new_profile)
await db.commit()
await db.refresh(new_profile)

return new_profile


@router.post(
"/accounts/activate/",
response_model=MessageResponseSchema,
status_code=status.HTTP_200_OK,
)
async def activate_account(
data: UserActivationRequestSchema,
background_tasks: BackgroundTasks,
db: AsyncSession = Depends(get_db),
email_sender: EmailSenderInterface = Depends(get_accounts_email_notificator),
):
stmt = (
select(ActivationTokenModel)
.join(UserModel)
.where(UserModel.email == data.email, ActivationTokenModel.token == data.token)
)
result = await db.execute(stmt)
activation_token = result.scalars().first()
if not activation_token:
raise HTTPException(
status_code=status.HTTP_404_NOT_FOUND,
detail="Invalid or expired activation token."
)

user = activation_token.user
user.is_active = True
await db.commit()

background_tasks.add_task(
email_sender.send_activation_complete_email,
email=user.email,
login_link="https://example.com/login",
)

return {"message": "User account activated successfully."}

Choose a reason for hiding this comment

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

This activate_account endpoint is a duplicate of the one found in src/routes/accounts.py. The task description indicates that routes/profiles.py should be for profile management, and account activation should be handled in routes/accounts.py. Please remove this duplicate endpoint from routes/profiles.py.

Loading
Loading