fix: data migration to delete Actions sans Assignments, and use on_delete=CASCADE #314

adamstankiewicz · 2023-11-01T20:41:37Z

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 to return obj.assignment.uuid where NoneType object has no attribute uuid (in get_assignment).

Includes:

Data migration to delete any Actions without an associated Assignment. Will leave any existing Actions with Assignments remaining to not lose any valid data.
Adjust the assignment field on the Actions model to include on_delete=models.CASCADE such that when Assignments get deleted, any associated Actions will also be deleted.
Adds created & modified fields to the LearnerContentAssignmentActionSerializer 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.
Being explicit about the order of Actions returned by the Assignments API (previously seemed to be non-deterministic).
Serializes an 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).
Ensures the has_reminded derived field only takes into account successful reminder actions. Similarly, ensures the has_notification derived field only takes into account successful email notifications.

…lete=CASCADE

adamstankiewicz · 2023-11-01T22:11:58Z

enterprise_access/apps/api/v1/tests/test_allocation_view.py

@@ -6,7 +6,6 @@

 import ddt
 from django.core.cache import cache as django_cache
-from django.core.exceptions import ValidationError


make quality was complaining about a duplicate import.

adamstankiewicz · 2023-11-01T22:13:07Z

.../apps/content_assignments/migrations/0010_alter_learnercontentassignmentaction_assignment.py

+    ]
+
+    operations = [
+        migrations.RunPython(delete_actions_without_assignment, migrations.RunPython.noop),


Run data migration before altering the assignments field below.

adamstankiewicz · 2023-11-01T22:14:49Z

enterprise_access/apps/content_assignments/models.py

+                    completed_at__isnull=False,
+                    error_reason__isnull=True,


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.

adamstankiewicz · 2023-11-01T22:15:02Z

enterprise_access/apps/content_assignments/models.py

+                    completed_at__isnull=False,
+                    error_reason__isnull=True,


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.

adamstankiewicz · 2023-11-01T22:15:52Z

.../subsidy_access_policy/migrations/0018_alter_subsidyaccesspolicy_assignment_configuration.py

@@ -0,0 +1,20 @@
+# Generated by Django 3.2.21 on 2023-11-01 20:35


This migration is generated with make migrations on the main branch without any code changes... Should it be committed?

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.

Oh, hmm, weird. FWIW, MySQL does support unique_together, it just doesn't support conditional unique constraints.

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.

Sounds good, thanks for the sanity check! Will keep this migration.

adamstankiewicz · 2023-11-01T22:16:22Z

enterprise_access/apps/subsidy_access_policy/models.py

@@ -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)


make quality suggested this pylint ignore comment wasn't necessary.

pwnage101

just a nit, looks great!

enterprise_access/apps/api/serializers/content_assignments/assignment.py

pwnage101 · 2023-11-02T01:56:02Z

.../subsidy_access_policy/migrations/0018_alter_subsidyaccesspolicy_assignment_configuration.py

@@ -0,0 +1,20 @@
+# Generated by Django 3.2.21 on 2023-11-01 20:35


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.

iloveagent57 · 2023-11-02T13:37:59Z

enterprise_access/apps/api/serializers/content_assignments/assignment.py

+        """
+        Resolves any associated actions to the assignment in chronological order based on created timestamp.
+        """
+        related_actions = assignment.actions.order_by('created').all()


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).

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

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!

iloveagent57 · 2023-11-02T13:42:07Z

enterprise_access/apps/content_assignments/models.py

+                    completed_at__isnull=False,
+                    error_reason__isnull=True,


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.

iloveagent57 · 2023-11-02T13:45:46Z

enterprise_access/apps/api/serializers/content_assignments/assignment.py

+            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()


nit: you don't need the .all() here, since it's already a filter().

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

adamstankiewicz added 5 commits November 1, 2023 16:41

fix: data migration to delete Actions sans Assignments, and use on_de…

34e70d6

…lete=CASCADE

fix: updates

7ed25db

chore: updates

7ccf9f6

fix: updates

61ef724

chore: updates

e12c5a0

adamstankiewicz commented Nov 1, 2023

View reviewed changes

chore: undo a change

e70cffe

adamstankiewicz marked this pull request as ready for review November 1, 2023 22:41

adamstankiewicz mentioned this pull request Nov 1, 2023

fix: rely on new error_reason field to determine failed learner_state reason openedx/frontend-app-admin-portal#1079

Merged

3 tasks

pwnage101 approved these changes Nov 2, 2023

View reviewed changes

adamstankiewicz mentioned this pull request Nov 2, 2023

fix: allow non-opaque-keys as content key for allocate requests #313

Merged

iloveagent57 reviewed Nov 2, 2023

View reviewed changes

chore: PR feedback

aae3d9e

adamstankiewicz force-pushed the ags/resolve-actions-django-admin-500 branch from 80e6fa1 to aae3d9e Compare November 2, 2023 14:37

chore: move CSRF_TRUSTED_ORIGINS out of CORS CONFIG section

f26fc04

adamstankiewicz requested a review from iloveagent57 November 2, 2023 14:40

iloveagent57 approved these changes Nov 2, 2023

View reviewed changes

adamstankiewicz merged commit 305ab26 into main Nov 2, 2023
4 checks passed

adamstankiewicz deleted the ags/resolve-actions-django-admin-500 branch November 2, 2023 14:58

adamstankiewicz mentioned this pull request Nov 2, 2023

chore: allow optional logging of Django's SQL #311

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: data migration to delete Actions sans Assignments, and use on_delete=CASCADE #314

fix: data migration to delete Actions sans Assignments, and use on_delete=CASCADE #314

adamstankiewicz commented Nov 1, 2023 •

edited

Loading

adamstankiewicz Nov 1, 2023

adamstankiewicz Nov 1, 2023

adamstankiewicz Nov 1, 2023

adamstankiewicz Nov 1, 2023

adamstankiewicz Nov 1, 2023

pwnage101 Nov 2, 2023

iloveagent57 Nov 2, 2023

iloveagent57 Nov 2, 2023

adamstankiewicz Nov 2, 2023

adamstankiewicz Nov 1, 2023

pwnage101 left a comment •

edited

Loading

pwnage101 Nov 2, 2023

iloveagent57 Nov 2, 2023

iloveagent57 Nov 2, 2023

adamstankiewicz Nov 2, 2023

iloveagent57 Nov 2, 2023

iloveagent57 Nov 2, 2023

		@@ -0,0 +1,20 @@
		# Generated by Django 3.2.21 on 2023-11-01 20:35

fix: data migration to delete Actions sans Assignments, and use on_delete=CASCADE #314

fix: data migration to delete Actions sans Assignments, and use on_delete=CASCADE #314

Conversation

adamstankiewicz commented Nov 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwnage101 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamstankiewicz commented Nov 1, 2023 •

edited

Loading

pwnage101 left a comment •

edited

Loading