Skip to content
Open
Show file tree
Hide file tree
Changes from all 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
12 changes: 12 additions & 0 deletions backend/plan/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,23 @@ class Schedule(models.Model):
"""
),
)

advanced_registration = models.BooleanField(
blank=False,
Copy link
Copy Markdown
Contributor

@AaDalal AaDalal Jan 5, 2023

Choose a reason for hiding this comment

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

Do not need because blank should be false by default

default=False,
help_text=dedent(
"""
A label to denote whether the user's schedule was made for advanced registration.
"""
),
)

created_at = models.DateTimeField(auto_now_add=True)
updated_at = models.DateTimeField(auto_now=True)

class Meta:
unique_together = (("name", "semester", "person"),)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think you need this.

unique_together = (("name", "semester", "person", "advanced_registration"), ("semester", "person", "advanced_registration"))
Copy link
Copy Markdown
Contributor

@AaDalal AaDalal Jan 5, 2023

Choose a reason for hiding this comment

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

Each tuple is anded together -- so both tuples have to be unique, otherwise everything fails. The first tuple ends up being redundant because it just a subset of the cases covered by the second tuple.

I would fix this by deleting the second constraint. When advanced registration is False (all of the cases prior to this PR, we have the original constraint of ("name", "semester", "person") which we want to enforce. When advanced_registration is true, we should only have 1 schedule per semester so the ("name", "semester", "person") constraint should always succeed.


def __str__(self):
return "User: %s, Schedule ID: %s" % (self.person, self.id)
4 changes: 2 additions & 2 deletions backend/plan/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,7 @@
from django.views.generic import TemplateView
from rest_framework import routers

from plan.views import ScheduleViewSet, recommend_courses_view

from plan.views import ScheduleViewSet, recommend_courses_view, advanced_registration_schedule_view

router = routers.DefaultRouter()
router.register(r"schedules", ScheduleViewSet, basename="schedules")
Expand All @@ -12,5 +11,6 @@
urlpatterns = [
path("", TemplateView.as_view(template_name="plan/build/index.html")),
path("recommendations/", recommend_courses_view, name="recommend-courses"),
path("advanced-registration/", advanced_registration_schedule_view, name="advanced-registration-schedule"),
path("", include(router.urls)),
]
86 changes: 86 additions & 0 deletions backend/plan/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,92 @@
},
)
)

# PcxAutoSchema for the advanced registration view.
@api_view(["POST"])
@schema(
PcxAutoSchema(
response_codes={
reverse_func("advanced-registration-schedule"): {
"POST": {
200: "[DESCRIBE_RESPONSE_SCHEMA]Response returned successfully.",
201: "[UNDOCUMENTED]",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there ever a case where 201 is returned?

400: "Invalid advanced_reg_codes, advanced_reg_sections, schedule_user, or advanced_reg_schedule_name (see response).",
}
}
},
override_request_schema={
reverse_func("advanced-registration-schedule"): {
"POST": {
"type": "object",
"properties": {
"advanced_reg_codes": {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Where is semester?

"type": "array",
"description": (
"An array of courses the user is currently planning to "
"take, each specified by its string full code, of the form "
"e.g. CIS-120-000."
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: don't need the "e.g." on this line

),
"items": {"type": "string"},
},
"advanced_reg_schedule_name": {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Assuming that we only allow 1 adv registration schedule per semester per user, does this make sense to include? Should the name just be something like "advanced registration"

"type": "string",
"description": (
"Name of the advanced registration schedule as "
"Specified by the user "
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Super tiny nit: specified and period at the end

),
},
},
}
}
},
override_response_schema={
reverse_func("advanced-registration-schedule"): {
"POST": {
200: {"type": "object"}
}
}
},
)
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would delete this space. Have you tried running the documentation locally to see how the auto docs look?

@permission_classes([IsAuthenticated])
def advanced_registration_schedule_view(request):
"""
This route will takes in advanced registration codes and a schedule's name
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe clarify what codes we're referring to here.

to compose an advanced registration schedule and save it to the database. This view
also returns the created schedule in the response.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would elaborate more on why this would return 400. For example (from recommend_courses_view(...))

    If successful, this route will return a list of recommended courses, with the same schema
    as the List Courses route, starting with the most relevant course. The number of
    recommended courses returned can be specified using the n_recommendations attribute in the
    request body, but if this attribute is omitted, the default will be 5.
    If n_recommendations is not an integer, or is <=0, a 400 will be returned.
    If curr_courses contains repeated courses or invalid courses or non-current courses, a
    400 will be returned.
    If past_courses contains repeated courses or invalid courses, a 400 will be returned.
    If curr_courses and past_courses contain overlapping courses, a 400 will be returned

"""

schedule_user = request.user
advanced_reg_codes = request.data.get("advanced_reg_codes", None)
advanced_reg_codes = advanced_reg_codes if advanced_reg_codes is not None else []
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You could simplify the two lines above as just advanced_reg_codes = request.data.get("advanced_reg_codes", [])


advanced_reg_schedule_name = request.data.get("advanced_reg_schedule_name")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What if this is not included in the request?

semester = request.data.get("semester", get_current_semester())

try:
advanced_reg_sections = []
for advanced_reg_code in advanced_reg_codes:
advanced_reg_sections.append(Section.objects.filter(code=advanced_reg_code))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can simplify this for loop into Section.objects.filter(code__in=advanced_reg_codes)

Copy link
Copy Markdown
Contributor

@AaDalal AaDalal Jan 5, 2023

Choose a reason for hiding this comment

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

One other important note: we only want the Section objects from the semester in question, right? So I think it should be Section.objects.filter(code__in=advanced_reg_codes, semester=semester)


if len(advanced_reg_sections) == 0:
return Response(
f"Empty Advanced Registration Schedule",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: unnecessary formatted string (remove the leading f)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe an error code would be No valid sections provided (or something along those lines)

status=status.HTTP_400_BAD_REQUEST,
)

advanced_reg_schedule = Schedule.objects.create(person=schedule_user, sections=advanced_reg_sections,
semester=semester, name=advanced_reg_schedule_name, advanced_registration=True)
advanced_reg_schedule.save()

except ValueError as e:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What causes this ValueError? It's generally better practice to avoid try-except clauses and instead ensure your inputs are correct before using them.

return Response(
str(e),
status=status.HTTP_400_BAD_REQUEST,
)
return Response({"message": "success", "advanced_reg_schedule": advanced_reg_schedule}, status=status.HTTP_200_OK)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does this response look correct? I'm actually unsure whether returning a django model instance automatically serializes it.


@permission_classes([IsAuthenticated])
def recommend_courses_view(request):
"""
Expand Down