Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LLSC-24: Scheduling API #18

Open
wants to merge 34 commits into
base: main
Choose a base branch
from
Open

LLSC-24: Scheduling API #18

wants to merge 34 commits into from

Conversation

sunbagel
Copy link
Collaborator

@sunbagel sunbagel commented Dec 21, 2024

Notion ticket link

LLSC-24 Scheduling API

Implementation description

  • Created a model for Schedules and TimeBlocks
  • A Schedule is composed of several TimeBlocks, which determine ranges in Time
  • create_schedule() takes in several time blocks, then creates a Schedule with default values + creates the time_blocks. Uses the ORM relationship mapping to automatically map the time_blocks to the respective schedule id

Steps to test

  1. Make a request to http://localhost:8000/schedules (Check FastAPI docs for example input)
    image

  2. Verify that a new Schedule was created, and that the new Time Blocks have that Schedule id as a field
    Schedule 13 is created:
    image
    3 TimeBlocks were created corresponding to Schedule 13:
    image

What should reviewers focus on?

  • If model and schema definitions are sensible

Checklist

  • My PR name is descriptive and in imperative tense
  • My commit messages are descriptive and in imperative tense. My commits are atomic and trivial commits are squashed or fixup'd into non-trivial commits
  • I have run the appropriate linter(s)
  • I have requested a review from the PL, as well as other devs who have background knowledge on this PR or who will be building on top of this PR

Copy link
Collaborator

@mslwang mslwang left a comment

Choose a reason for hiding this comment

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

Looks great so far! Left some comments, but as we went through before, functionality wise this looks good to me.

backend/app/interfaces/schedule_service.py Outdated Show resolved Hide resolved
from abc import ABC, abstractmethod
from app.schemas.schedule import ScheduleCreate, ScheduleInDB, ScheduleAdd, ScheduleData, ScheduleRemove

class IScheduleService(ABC):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now that I think about this, I don't think we need this abstract class. ScheduleService is a standalone class.

from sqlalchemy.orm import Session

from app.models import Schedule, TimeBlock
# from app.schemas.schedule import UserCreate, UserInDB, UserRole
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should remove these comments!

backend/migrations/env.py Outdated Show resolved Hide resolved
from .Base import Base


class ScheduleState(Base):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's rename this to ScheduleStatus, I think this would be more indicative of a progession since Schedule is general.


class ScheduleState(str, Enum):
PENDING_PARTICIPANT = "PENDING_PARTICIPANT_RESPONSE"
PENDING_VOLUNTEER = "PENDING_VOLUNTEER_RESPONSE"
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: keep these in the same order as in the DB, just in case someone uses this as a reference for the ordering.

model_config = ConfigDict(from_attributes=True)

# Provides both Schedule data and full TimeBlock data
class ScheduleData(ScheduleInDB):
Copy link
Collaborator

Choose a reason for hiding this comment

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

In this case I'd call this ScheduleGetResponse.

@@ -20,7 +20,7 @@
@asynccontextmanager
async def lifespan(_: FastAPI):
log.info("Starting up...")
models.run_migrations()
# models.run_migrations()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Uncomment this line for now, we should have a separate PR to handle updating the migration steps.

'''
pass

class TimeBlockInDB(BaseModel):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Call this TimeBlockEntity instead, InDB isn't standard for DB representations.

backend/test.db Outdated Show resolved Hide resolved
@mslwang
Copy link
Collaborator

mslwang commented Dec 31, 2024

Reminder to fix linter rules as well

sunbagel and others added 5 commits February 4, 2025 20:50
update schedule_state to schedule_status
## Notion ticket link
<!-- Please replace with your ticket's URL -->
[Ticket
Name](https://www.notion.so/uwblueprintexecs/Task-Board-db95cd7b93f245f78ee85e3a8a6a316d)


<!-- Give a quick summary of the implementation details, provide design
justifications if necessary -->
## Implementation description
* 


<!-- What should the reviewer do to verify your changes? Describe
expected results and include screenshots when appropriate -->
## Steps to test
1.


<!-- Draw attention to the substantial parts of your PR or anything
you'd like a second opinion on -->
## What should reviewers focus on?
* 


## Checklist
- [ ] My PR name is descriptive and in imperative tense
- [ ] My commit messages are descriptive and in imperative tense. My
commits are atomic and trivial commits are squashed or fixup'd into
non-trivial commits
- [ ] I have run the appropriate linter(s)
- [ ] I have requested a review from the PL, as well as other devs who
have background knowledge on this PR or who will be building on top of
this PR
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants