-
Notifications
You must be signed in to change notification settings - Fork 5
Advanced Registration View #446
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,11 +43,23 @@ class Schedule(models.Model): | |
| """ | ||
| ), | ||
| ) | ||
|
|
||
| advanced_registration = models.BooleanField( | ||
| blank=False, | ||
| 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"),) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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")) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Each tuple is 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 |
||
|
|
||
| def __str__(self): | ||
| return "User: %s, Schedule ID: %s" % (self.person, self.id) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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]", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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": { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where is |
||
| "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." | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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": { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 " | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Super tiny nit: |
||
| ), | ||
| }, | ||
| }, | ||
| } | ||
| } | ||
| }, | ||
| override_response_schema={ | ||
| reverse_func("advanced-registration-schedule"): { | ||
| "POST": { | ||
| 200: {"type": "object"} | ||
| } | ||
| } | ||
| }, | ||
| ) | ||
| ) | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| """ | ||
|
|
||
| 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 [] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could simplify the two lines above as just |
||
|
|
||
| advanced_reg_schedule_name = request.data.get("advanced_reg_schedule_name") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can simplify this for loop into
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One other important note: we only want the |
||
|
|
||
| if len(advanced_reg_sections) == 0: | ||
| return Response( | ||
| f"Empty Advanced Registration Schedule", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: unnecessary formatted string (remove the leading f)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe an error code would be |
||
| 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: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What causes this |
||
| 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) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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): | ||
| """ | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not need because
blankshould be false by default