-
Notifications
You must be signed in to change notification settings - Fork 9
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
fix: data migration to delete Actions sans Assignments, and use on_delete=CASCADE #314
Conversation
@@ -6,7 +6,6 @@ | |||
|
|||
import ddt | |||
from django.core.cache import cache as django_cache | |||
from django.core.exceptions import ValidationError |
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.
make quality
was complaining about a duplicate import.
] | ||
|
||
operations = [ | ||
migrations.RunPython(delete_actions_without_assignment, migrations.RunPython.noop), |
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.
Run data migration before altering the assignments
field below.
completed_at__isnull=False, | ||
error_reason__isnull=True, |
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.
Before, this logic would consider reminder actions even when they have an error_reason
and/or have not been marked as "completed" via the completed_at
field.
completed_at__isnull=False, | ||
error_reason__isnull=True, |
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.
Before, this logic would consider notified actions even when they have an error_reason
and/or have not been marked as "completed" via the completed_at
field.
@@ -0,0 +1,20 @@ | |||
# Generated by Django 3.2.21 on 2023-11-01 20:35 |
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 migration is generated with make migrations
on the main
branch without any code changes... Should it be committed?
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 feel like this missing migration removes the field-level unique constraint that was supposed to be removed as part of #258. I'd say keeping this migration makes the database constraints more closely match the field definition.
Of course now we also know that mysql doesn't actually support the unique_together
that was supposed to replace the field-level constraint. But maybe that's a problem for a separate ticket.
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.
Oh, hmm, weird. FWIW, MySQL does support unique_together
, it just doesn't support conditional unique constraints.
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.
Here's the current UNIQUE keys on the table in our production environment:
| subsidy_access_policy_subsidyaccesspolicy | CREATE TABLE `subsidy_access_policy_subsidyaccesspolicy` (
`created` datetime(6) NOT NULL,
`modified` datetime(6) NOT NULL,
`policy_type` varchar(64) NOT NULL,
`uuid` char(32) NOT NULL,
...
PRIMARY KEY (`uuid`),
UNIQUE KEY `assignment_configuration_id` (`assignment_configuration_id`),
UNIQUE KEY `subsidy_access_policy_su_active_assignment_config_f6b904ce_uniq` (`active`,`assignment_configuration_id`),
...
So the composite key is there, but so is the single-column unique key, which we don't want. So yes, follow Troy's suggestion and let this migration go in.
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.
Sounds good, thanks for the sanity check! Will keep this migration.
@@ -274,7 +274,7 @@ def __new__(cls, *args, **kwargs): | |||
except Exception: # pylint: disable=broad-except | |||
pass | |||
|
|||
return super().__new__(proxy_class) # pylint: disable=lost-exception | |||
return super().__new__(proxy_class) |
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.
make quality
suggested this pylint ignore comment wasn't necessary.
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.
just a nit, looks great!
enterprise_access/apps/api/serializers/content_assignments/assignment.py
Show resolved
Hide resolved
@@ -0,0 +1,20 @@ | |||
# Generated by Django 3.2.21 on 2023-11-01 20:35 |
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 feel like this missing migration removes the field-level unique constraint that was supposed to be removed as part of #258. I'd say keeping this migration makes the database constraints more closely match the field definition.
Of course now we also know that mysql doesn't actually support the unique_together
that was supposed to replace the field-level constraint. But maybe that's a problem for a separate ticket.
""" | ||
Resolves any associated actions to the assignment in chronological order based on created timestamp. | ||
""" | ||
related_actions = assignment.actions.order_by('created').all() |
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.
Tiniest possible nit: it's more conventional to put the all()
before the order_by()
(because if you were writing it as raw SQL, the ORDER BY
would come last).
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.
BTW, there's a whole other way to do this, which is to add an ordering
to the Action model's meta class. https://docs.djangoproject.com/en/4.2/ref/models/options/#ordering
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.
Oh, nice. Didn't realize there was a shorthand way to do this via the Meta
class (TIL)! It's a cleaner solution, will update. Thanks!
completed_at__isnull=False, | ||
error_reason__isnull=True, |
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.
Another tiniest possible nit: can we switch this so that the error reason filter comes before the completed_at filter? That way, it'll match the filter order above.
return None | ||
|
||
# Assignment is an errored state, so determine the appropriate error reason so clients don't need to. | ||
related_actions_with_error = assignment.actions.filter(error_reason__isnull=False).order_by('-created').all() |
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: you don't need the .all()
here, since it's already a filter()
.
80e6fa1
to
aae3d9e
Compare
fix: refactored code based on reviewer feedback fix: allow non-opaque-keys as content key for allocate requests fix: data migration to delete Actions sans Assignments, and use on_delete=CASCADE (#314) chore: allow optional logging of Django's SQL fix: missing content titles on assignments feat: Try to populate lms_user_id on assignments during creation This is an attempt to cover the case where learners are already logged in at the moment of assignment. This case is a hole left by the work in ENT-7875 which only set out to cover the case where a learner was logged out at the moment of assignment. ENT-7874 feat: Added assignments in credits_available endpoint feat: allowing consumers of the learner content assignment api to filter by email and content title fix: lint error chore: refactored fix: updated failing tests feat: fetch enterprise catalog content metadata, serialize for credits_available assignments ENT-7878 feat: add course_type field to assignment content metadata serializer feat: return `learner_state_counts` as part of the admin assignments list API response (#322) feat: validate requested allocation price feat: link learners during assignment allocation ENT-7778 | Call task to link learners to enterprise customer during allocation, and have that ask add a successful linked action. fix: new approach to generate `learner_state_count` in admin assignments list API; add addtl ordering and filtering support (#324) chore: rebase fix: lint error chore: added test coverage
https://enterprise-access-internal.stage.edx.org/admin/content_assignments/learnercontentassignmentaction/ currently throwing a
Server Error (500)
due to Actions that exist without associated Assigments, which makes the Django Admin screen throw an exception due toreturn obj.assignment.uuid
whereNoneType
object has no attributeuuid
(inget_assignment
).Includes:
assignment
field on the Actions model to includeon_delete=models.CASCADE
such that when Assignments get deleted, any associated Actions will also be deleted.created
&modified
fields to theLearnerContentAssignmentActionSerializer
so clients have more visibility into when these actions occurred, notably for when they are not successful (completed_at=None
). Will also be useful to help debug the actions ordering further to QA the "Status" column in the "Assigned" table.error_reason
field in the Assignments API so the frontend doesn't have to parse the actions to find the appropriate reason why the assignment is in an errored state).