Skip to content

feat: add new field for Content-/UserDate models to support assignments #324

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.

Sign up for GitHub

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

Closed
andrii-hantkovskyi wants to merge 3 commits into from
Closed

feat: add new field for Content-/UserDate models to support assignments #324

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
582089c
feat: add new field for Content-/UserDate models to support assignments
Jun 23, 2025
a15bbd5
fix: remove user_date_value, recreate 0009 migration
Serj-N Aug 20, 2025
9f4308a
fix: fix imports order in 0009 migration
Serj-N Aug 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions edx_when/migrations/0009_contentdate_assignment_title_contentdate_course_name_and_more.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
# Generated by Django 4.2.22 on 2025-08-20 09:11

import opaque_keys.edx.django.models
from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
('edx_when', '0008_courseversion_block_type'),
]

operations = [
migrations.AddField(
model_name='contentdate',
name='assignment_title',
Copy link
Contributor

@e0d e0d Aug 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assignment feels like a pretty robust concept, perhaps it should eventually be a new model entirely. How do we expect the assignment_title to be calculated?

Copy link
Contributor

@Serj-N Serj-N Aug 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, I'm stepping in for @andrii-hantkovskyi.
assignment_title is computed in the get_course_assignments() function, in a loop over course blocks:
title = block_data.get_xblock_field(subsection_key, 'display_name', _('Assignment'))

field=models.CharField(blank=True, db_index=True, default='', max_length=255),
),
migrations.AddField(
model_name='contentdate',
name='course_name',
field=models.CharField(blank=True, default='', max_length=255),
),
migrations.AddField(
model_name='contentdate',
name='subsection_name',
field=models.CharField(blank=True, db_index=True, default='', max_length=255),
),
migrations.AddField(
Copy link
Contributor

@e0d e0d Aug 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is the first instance of content data in the userdate model. Why do we need to introduce it here?

Copy link
Contributor

@Serj-N Serj-N Aug 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first_component_block_id field will be included in the response of the "all-course-dates" api endpoint.
Calculating this value in runtime is not trivial, and having it here on UserDate makes it much easier.

Copy link
Contributor

@e0d e0d Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this specific to the user or would it be the same for each ContentDate? How does it different or related to the location field in ContentDate.

Copy link

@cmltaWt0 cmltaWt0 Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This field should not have a User-related context (unless I miss some business requirements) and must be placed in the ContentDate. I believe @e0d is totally right.

I want to perf-test the course info API w/ and w/o this field to check on the clear necessity to add it here.

Copy link
Contributor

@Serj-N Serj-N Sep 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The location attribute on ContentDate is the key of the assignment block itself, and it is not user-specific.
The first_component_block_id attribute on UserDate is the key of the first unit of the block available to the user, which means it is user-specific.
Here is a possible scenario that may help explain it:

  1. We have a subsection with Unit 1 followed by Unit 2 followed by Unit 3.
  2. Two users - User A and User B - have access to this subsection as an assignment.
  3. However, User A belongs to a cohort that does not have access to Unit 1.
  4. In this case, ContentDate.location will be the same for the two users (the subsection), but UserDate.first_component_block_id will differ: Unit 1 for User B, but Unit 2 for User A.

That's the logic behind putting first_component_block_id on the UserDate model.

Copy link

@cmltaWt0 cmltaWt0 Sep 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Should/can we rename the field name then (the current naming is really misleading)? To something like first_available_block_id.

Copy link
Contributor

@Serj-N Serj-N Sep 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should: this naming was not introduced by this PR - it is already used extensively throughout the codebase.

Copy link
Contributor

@Serj-N Serj-N Sep 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cmltaWt0 @e0d The following code should help clarify the meaning of first_component_block_id and why it is considered user-specific in the existing codebase. In the get_course_assignments() function we have these lines (abbreviated):

block_data = get_course_blocks(user, course_usage_key, allow_start_dates_in_future=True, include_completion=True)
...
for section_key in block_data.get_children(course_usage_key):
    for subsection_key in block_data.get_children(section_key):
            ...
            first_component_block_id = get_first_component_of_block(subsection_key, block_data)

As we can see, the value of first_component_block_id is based on the course structure (block_data) - which is collected for a specfic user and only includes what they have access to.

model_name='userdate',
name='first_component_block_id',
field=opaque_keys.edx.django.models.UsageKeyField(blank=True, db_index=True, max_length=255, null=True),
),
migrations.AddIndex(
model_name='contentdate',
index=models.Index(fields=['assignment_title', 'course_id'], name='edx_when_assignment_course_idx'),
),
migrations.AddIndex(
model_name='contentdate',
index=models.Index(fields=['subsection_name', 'course_id'], name='edx_when_subsection_course_idx'),
),
migrations.AddIndex(
model_name='userdate',
index=models.Index(fields=['user', 'first_component_block_id'], name='edx_when_user_first_block_idx'),
),
]
30 changes: 30 additions & 0 deletions edx_when/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,13 +93,18 @@ class ContentDate(models.Model):
field = models.CharField(max_length=255, default='')
active = models.BooleanField(default=True)
block_type = models.CharField(max_length=255, null=True)
assignment_title = models.CharField(max_length=255, blank=True, default='', db_index=True)
course_name = models.CharField(max_length=255, blank=True, default='')
subsection_name = models.CharField(max_length=255, blank=True, default='', db_index=True)

class Meta:
"""Django Metadata."""

unique_together = ('policy', 'location', 'field')
indexes = [
models.Index(fields=('course_id', 'block_type'), name='edx_when_course_block_type_idx'),
models.Index(fields=('assignment_title', 'course_id'), name='edx_when_assignment_course_idx'),
models.Index(fields=('subsection_name', 'course_id'), name='edx_when_subsection_course_idx'),
]

def __str__(self):
Expand All @@ -109,6 +114,14 @@ def __str__(self):
# Location already holds course id
return f'ContentDate({self.policy}, {self.location}, {self.field}, {self.block_type})'

def __repr__(self):
"""
Get a detailed representation of this model instance.
"""
return (f'ContentDate(id={self.id}, assignment_title="{self.assignment_title}", '
f'course_name="{self.course_name}", subsection_name="{self.subsection_name}", '
f'policy={self.policy}, location={self.location})')


class UserDate(TimeStampedModel):
"""
Expand All @@ -125,6 +138,14 @@ class UserDate(TimeStampedModel):
actor = models.ForeignKey(
get_user_model(), null=True, default=None, blank=True, related_name="actor", on_delete=models.CASCADE
)
first_component_block_id = UsageKeyField(null=True, blank=True, max_length=255, db_index=True)

class Meta:
"""Django Metadata."""

indexes = [
models.Index(fields=('user', 'first_component_block_id'), name='edx_when_user_first_block_idx'),
]

@property
def actual_date(self):
Expand Down Expand Up @@ -169,3 +190,12 @@ def __str__(self):
# Location already holds course id
# pylint: disable=no-member
return f'{self.user.username}, {self.content_date.location}, {self.content_date.field}'

def __repr__(self):
"""
Get a detailed representation of this model instance.
"""
# pylint: disable=no-member
return (f'UserDate(id={self.id}, user="{self.user.username}", '
f'first_component_block_id={self.first_component_block_id}, '
f'content_date={self.content_date.id})')