Advanced Registration View#446
Conversation
Advanced registration view changes for Penn Mobile
|
Unlinted copy of the PR for the advanced registration view |
There was a problem hiding this comment.
Good work here, but there are a few big things to handle before we merge:
- Do we want to allow more than 1 advanced registration schedule per user per semester? If so, why? If not, we need to preserve that invariant, probably by overriding the
savemethod ofSchedule. - How can we make sure that the inputs to the view are properly validated?
Couple of general notes:
- IMPORTANT: whenever you make changes to a model, you should run
makemigrationsandmigrateand commit the migrations file that produces. I don't see the migration here, which could be problematic! - IMPORTANT: you need to test your code using the unit testing framework (see the
backend/testsfolder) - make the description of the pull request more descriptive (see some of our previous PRs, particular @mureytasroc's for inspo).
- The title can similarly more clearly describe what this feature adds.
| ) | ||
|
|
||
| advanced_registration = models.BooleanField( | ||
| blank=False, |
There was a problem hiding this comment.
Do not need because blank should be false by default
| updated_at = models.DateTimeField(auto_now=True) | ||
|
|
||
| class Meta: | ||
| unique_together = (("name", "semester", "person"),) |
There was a problem hiding this comment.
I don't think you need this.
|
|
||
| class Meta: | ||
| unique_together = (("name", "semester", "person"),) | ||
| unique_together = (("name", "semester", "person", "advanced_registration"), ("semester", "person", "advanced_registration")) |
There was a problem hiding this comment.
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.
| }, | ||
| ) | ||
| ) | ||
|
|
There was a problem hiding this comment.
I would delete this space. Have you tried running the documentation locally to see how the auto docs look?
| reverse_func("advanced-registration-schedule"): { | ||
| "POST": { | ||
| 200: "[DESCRIBE_RESPONSE_SCHEMA]Response returned successfully.", | ||
| 201: "[UNDOCUMENTED]", |
There was a problem hiding this comment.
Is there ever a case where 201 is returned?
|
|
||
| if len(advanced_reg_sections) == 0: | ||
| return Response( | ||
| f"Empty Advanced Registration Schedule", |
There was a problem hiding this comment.
Nit: unnecessary formatted string (remove the leading f)
| try: | ||
| advanced_reg_sections = [] | ||
| for advanced_reg_code in advanced_reg_codes: | ||
| advanced_reg_sections.append(Section.objects.filter(code=advanced_reg_code)) |
There was a problem hiding this comment.
can simplify this for loop into Section.objects.filter(code__in=advanced_reg_codes)
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
Maybe an error code would be No valid sections provided (or something along those lines)
| semester=semester, name=advanced_reg_schedule_name, advanced_registration=True) | ||
| advanced_reg_schedule.save() | ||
|
|
||
| except ValueError as e: |
There was a problem hiding this comment.
What causes this ValueError? It's generally better practice to avoid try-except clauses and instead ensure your inputs are correct before using them.
| str(e), | ||
| status=status.HTTP_400_BAD_REQUEST, | ||
| ) | ||
| return Response({"message": "success", "advanced_reg_schedule": advanced_reg_schedule}, status=status.HTTP_200_OK) |
There was a problem hiding this comment.
Does this response look correct? I'm actually unsure whether returning a django model instance automatically serializes it.
2 participants
Added linting changes to the advanced registration view