AA-34: Add UserCalendar Model - #23147
Conversation
|
jenkins run bokchoy |
| from opaque_keys.edx.django.models import CourseKeyField | ||
|
|
||
|
|
||
| class UserCalendar(models.Model): |
There was a problem hiding this comment.
Should this have a uniqueness constraint of (user, course_key)?
There was a problem hiding this comment.
I think so, yes. Otherwise a user could have multiple calendar configs for the same course.
| from opaque_keys.edx.django.models import CourseKeyField | ||
|
|
||
|
|
||
| class UserCalendar(models.Model): |
There was a problem hiding this comment.
I think so, yes. Otherwise a user could have multiple calendar configs for the same course.
| .. no_pii: | ||
| """ | ||
| user = models.ForeignKey(User, db_index=True, on_delete=models.CASCADE) | ||
| course_key = CourseKeyField(max_length=255, db_index=True) |
There was a problem hiding this comment.
Ugh. Do we still do this, and avoid foreign keys?
There was a problem hiding this comment.
This was the pattern I found throughout the code. Not sure what we would want to foreign key against anyway
| """ | ||
| user = models.ForeignKey(User, db_index=True, on_delete=models.CASCADE) | ||
| course_key = CourseKeyField(max_length=255, db_index=True) | ||
| enabled = models.BooleanField(default=False) |
There was a problem hiding this comment.
Maybe worth adding a utility method like is_enabled_for_course(user, course) that does a get with a default value of False if it doesn't exist in the model?
| from opaque_keys.edx.django.models import CourseKeyField | ||
|
|
||
|
|
||
| class UserCalendar(models.Model): |
There was a problem hiding this comment.
Should this be named UserCalendarConfig or something? Rather than it being the object itself?
Or even, UserCalendarSyncConfig, because this is toggling the sync/email functionality. Or if not that, maybe rename enabled to sync_enabled or something.
49f3f44 to
4d883b3
Compare
There was a problem hiding this comment.
This is 2 lookups, yeah? Try/except might get you to just one.
There was a problem hiding this comment.
You got it dude
87155b9 to
a3093e2
Compare
a3093e2 to
bb1981d
Compare
|
Your PR has finished running tests. There were no failures. |
|
EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production. |
|
EdX Release Notice: This PR has been deployed to the production environment. |
4 participants
No description provided.