-
Notifications
You must be signed in to change notification settings - Fork 2
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
Class announcements #26
Conversation
dffd64c
to
0b05b8b
Compare
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.
Very good work in general implementing this functionality. See comments in files.
I have one major architecture comment regarding your ClassGroup
model. This implementation seems very "hacky" to me, with the check for the username prefix and "_" for everyone. I would suggest you replace this using Django's built-in Group
model and rewrite the logic accordingly. In addition, whenever a user logs in, the following pseudocode could be executed:
group = Group.objects.get(name=user.graduation_year)
if not group.exists():
Group.objects.create(name=user.graduation_year)
else:
if group not in user.groups().all():
user.groups.add(group)
This would also allow for custom groups consisting of certain groups of users, such as SGA or CC. This also removes the need to manually add class groups every year.
Please also squash your commits to preserve only those with clearly defined feature additions.
198cbaf
to
66fcdec
Compare
66fcdec
to
0051f86
Compare
I am merging this PR now since the proposed architecture is simple to migrate to and is not affected by the current deployment implementation. Further changes to the features implemented in this PR will be made directly to the master branch. |
Adds functionality for:
Still needed: