-
Notifications
You must be signed in to change notification settings - Fork 0
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
[BB-9570] Implement Learning Path Enrollment API #11
base: master
Are you sure you want to change the base?
Conversation
@Agrendalath Hi, the CI fails due to missing PII annotation for the "HistoricalRecords" of the |
'unique_together': {('email', 'learning_path')}, | ||
}, | ||
), | ||
] |
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.
Missing operations:
operations = [
migrations.AlterModelOptions(
name="historicallearningpathenrollment",
options={
"get_latest_by": "history_date",
"ordering": ("-history_date", "-history_id"),
"verbose_name": "historical learning path enrollment",
},
),
migrations.AlterField(
model_name="historicallearningpathenrollment",
name="enrolled_at",
field=models.DateTimeField(
default=datetime.datetime(
2025, 3, 16, 12, 5, 48, 376085, tzinfo=datetime.timezone.utc
),
help_text="Timestamp of enrollment or un-enrollment. To be explicitly set when performing a learner enrollment.",
),
),
migrations.AlterField(
model_name="learningpathenrollment",
name="enrolled_at",
field=models.DateTimeField(
default=datetime.datetime(
2025, 3, 16, 12, 5, 48, 376085, tzinfo=datetime.timezone.utc
),
help_text="Timestamp of enrollment or un-enrollment. To be explicitly set when performing a learner enrollment.",
),
),
]
Please squash them into 0006.
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.
This is strange. I think this might be due to the difference in the versions of "django-simple-history" between our environments. My redwood environment has been broken and the openedx-dev image keeps failing to build. So, I used the sumac branch for testing.
I will try to get the redwood up again and regenerate the migrations.
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.
@tecoholic, you don't need to use Redwood for this. I see that all Open edX requirements from Redwood to the latest version specify django-simple-history==3.4.0
, while the common constraints contain the following:
# django-simple-history>3.0.0 adds indexing and causes a lot of migrations to be affected
django-simple-history==3.0.0
It might be worth taking a look at the context of this constraint, as we typically don't want to have a version mismatch between the CI and instance.
learning_paths/api/v1/views.py
Outdated
if username: | ||
q.filter(user__username=username) | ||
|
||
if not request.user.is_staff and not request.user.is_superuser: | ||
if username and request.user.username != username: | ||
raise PermissionDenied |
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.
This pattern is repeated in a few places. Why don't we create the has_permission method that will return username
if it exists and the request.user
is staff
or superuser
. Otherwise, return request.user.username
.
Also, please check if it doesn't raise 500 for AnonymousUser
.
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.
@Agrendalath This comment threw me off course for a while, before I realized that has_permission
can return only True/False. This was a great suggestion nonetheless.
After a couple of attempts, I think I finally got a decent one implemented. I had also overdone the admin checks by checking for both is_staff
and is_superuser
in all the places. I was able to simplify that as well.
learning_paths/api/v1/urls.py
Outdated
LearningPathEnrollmentView.as_view(), | ||
name="learning-path-enrollments", | ||
), | ||
path("enrollments/", FetchEnrollmentsView.as_view(), name="fetch-enrollments"), |
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.
As you have noted in the ticket, we should update all URLs to be prefixed with learning-paths
for consistency.
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.
user=user, learning_path=learning_path | ||
).first() | ||
enrolled_now = False | ||
if not enrollment: | ||
enrollment = LearningPathEnrollment( | ||
user=user, | ||
learning_path=learning_path, | ||
is_active=True, | ||
enrolled_at=datetime.now(timezone.utc), | ||
) | ||
enrolled_now = True | ||
if not enrollment.is_active: | ||
enrollment.is_active = True | ||
enrollment.enrolled_at = datetime.now(timezone.utc) | ||
enrolled_now = True | ||
enrollment.save() | ||
if enrolled_now: | ||
enrollments_created.append(enrollment) |
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.
Nit: why don't we use update_or_create for all of this?
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.
@Agrendalath Good question. There is an edge case. When there is an active enrollment for the learning update_or_create
will change the "enrolled_at" date of the enrollment. I couldn't find a way to set is_active
for inactive enrollments, but not change enrolled_at
for active enrollments.
# Create LearningPathEnrollmentAllowed for non-existing users | ||
for email in non_existing_emails: | ||
try: | ||
validate_email(email) |
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.
Nit: won't creating LearningPathEnrollmentAllowed
automatically raise the exception? The EmailField
already specifies this validator.
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.
I checked and even have an unit-test to verify this. Unfortunately, invalid emails don't seem to raise an error.
- Extracted the permission class to handle the is_staff check and verifying request.user matches the username passed. - Remove the "is_superuser" extra checks - Moved the username from query params to request.data for POST and DELETE requests - Updated the tests to reflect the above changes
Co-authored-by: Piotr Surowiec <[email protected]>
Co-authored-by: Piotr Surowiec <[email protected]>
Description
This PR implements the changes proposed in ADR 003.
List of changes introduced
Testing instructions
Setting up the plugin in a turor env
Testing the API