Archiving #270
Archiving #270
Conversation
…feat/archiving/main
…levant models inherit it
…feat/archiving/main
… by properties. removed all cached properties
… semester model) to sid to avoid keyword
…feat/archiving/model-updates
There was a problem hiding this comment.
In all, this looks really good; the model managers look good, and the semester object overall is pretty good (the fields could be tweaked slightly though).
In testing though, I've found a couple bugs. First, the pytest tests are failing because the Course factory has not been updated with the Semester object; this should be changed to ensure that pytest is still working. Some additional tests should probably be added to ensure that the distinguishing of semesters is respected with the queries.
Another bug I've found is that in accessing the section from the UI, an internal error occurs when accessing the Spacetime.objects attribute. This could be because it hasn't been overridden, though it seems a little bit weird that the base class would stop working; it'd be good to look into more.
In all, make sure that the pytest tests are all passing after each commit (at least, after each commit with finished features), to ensure that no regressions happen. This can be done locally with just pytest csm_web, and should run relatively quickly too. If possible, adding more tests would be great, to ensure that the functionality is still working as desired, and to check for any edge cases, though I know that's a work in progress still.
csm_web/scheduler/factories.py
Outdated
| @@ -278,4 +278,5 @@ def generate_test_data(preconfirm=False): | |||
| val = random.randint(1, 100) | |||
| ResourceFactory.create(course=course, week_num=i+1, date=date, topics=f"Topic {val}") | |||
| print("Done generating resources") | |||
|
|
|||
There was a problem hiding this comment.
Unsure why there's a newline here; likely a leftover from another reverted change? In general, it's always good to do a quick git status and a git diff to check over all your changes before committing just to avoid these kinds of extraneous diffs.
| Course = apps.get_model("scheduler", "Course") | ||
| Semester = apps.get_model("scheduler", "Semester") | ||
| initial_semester = Semester() | ||
| initial_semester.sid = 1 |
There was a problem hiding this comment.
It shouldn't be necessary to specify this value on creation; it should be automatically assigned to be the next available integer value.
csm_web/scheduler/models.py
Outdated
| @@ -58,6 +62,8 @@ class ValidatingModel(models.Model): | |||
| This abstract class fixes that insanity | |||
| """ | |||
|
|
|||
| past_objects_included = models.Manager() | |||
There was a problem hiding this comment.
I would perhaps rename this to all_objects just to be briefer, but it's up to you; it's just a preference thing.
csm_web/scheduler/models.py
Outdated
| @@ -66,6 +72,22 @@ class Meta: | |||
| abstract = True | |||
|
|
|||
|
|
|||
| class Semester(ValidatingModel): | |||
| sid = models.PositiveSmallIntegerField(primary_key=True) | |||
There was a problem hiding this comment.
A PositiveSmallIntegerField requires the user to manually specify the value; this isn't very optimal, since the user would need to know which integer values have been used and have not been used so far. An AutoField would be best here (I believe it is also the default for the primary key), as it is automatically specified on creation.
| @@ -66,6 +72,22 @@ class Meta: | |||
| abstract = True | |||
|
|
|||
|
|
|||
| class Semester(ValidatingModel): | |||
There was a problem hiding this comment.
I'd also add a field for the user-readable name of the semester, since currently it's only defined by the sid, which is not related to the meaning/interpretation of the semester object. This could just be a small CharField, or something similar; I'm envisioning it to just hold something like sp22 or fa20, etc. You could even add some validation to ensure that it's of the form (sp|fa)\d\d if you'd like.
csm_web/scheduler/serializers.py
Outdated
| @@ -1,7 +1,7 @@ | |||
| from rest_framework import serializers | |||
| from enum import Enum | |||
| from django.utils import timezone | |||
| from .models import Attendance, Course, SectionOccurrence, User, Student, Section, Mentor, Override, Spacetime, Coordinator, DayOfWeekField, Resource, Worksheet | |||
| from .models import Attendance, Course, SectionOccurrence, User, Student, Section, Mentor, Override, Spacetime, Coordinator, DayOfWeekField, Resource, Worksheet, Semester | |||
There was a problem hiding this comment.
It seems like the Semester model is imported here without ever being used.
| objects = models.Manager() | ||
|
|
||
| def __str__(self): | ||
| return f"{self.sid}" |
There was a problem hiding this comment.
If you do end up adding the extra description field, that should be added here to describe the semester model a little bit more.
…, updated migration
| @@ -1,7634 +1,8 @@ | |||
| { | |||
| "name": "csm_web", | |||
| "version": "1.0.0", | |||
| "lockfileVersion": 2, | |||
| "lockfileVersion": 1, | |||
There was a problem hiding this comment.
It seems like you have an outdated version of node, which changes the lockfile version here; update your node and revert these changes.
Changes only to models.py. Includes:
current_semester_sid()function outside of any class to clean up filterspast_objects_included, an alternative to objects, that includes all objects in the db across all semesters in a query.sid. More recent semesters have higher sid's. By default, all queries to other objects will filter for objects only from the most recent a.k.a. current semester. Because of highlight 2 (above), any query can replaceobjectswithpast_objects_includedto bypass this filter. (i.e.Student.objects.all()-> queryset of current students,Student.past_objects_included.all()-> queryset of students from all semesters in db).semesterfield. All other models are connected to a semester via the course model.Note: When first implementing these changes, every course will have its
semesterfield set toNone, and queries will error. This will be resolved as soon as a semester object is created for the current semester and each course's semester field is assigned that semester.