Skip to content
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

feat: failed notifications for assignments leave state as allocated #552

Merged
merged 1 commit into from
Sep 4, 2024

Conversation

iloveagent57
Copy link
Contributor

@iloveagent57 iloveagent57 commented Sep 4, 2024

Description

When the assignment email notification task fails, the corresponding assignment is now NOT moved to an errored state, but instead left in an allocated state. Furthermore, the admin-serializer for the assignment now indicates a "failed" learner state, and an error reason related to the failed notification attempt.
ENT-9037

Manual testing

Here's the new response from the serializer after an assignment was allocated, but the resulting notification task failed:

{
    "next": null,
    "previous": null,
    "count": 1,
    "num_pages": 1,
    "current_page": 1,
    "start": 0,
    "results": [
        {
            "uuid": "fdaeb538-2d9d-4875-87ca-2973754ed949",
            "assignment_configuration": "a9f97992-4894-4b80-9dde-b08ce3d7bff3",
            "learner_email": "test-failed-notification@example.com",
            "lms_user_id": null,
            "content_key": "edx+DemoX",
            "content_title": "Demonstration Course",
            "content_quantity": -14800,
            "state": "allocated",
            "transaction_uuid": null,
            "actions": [
                {
                    "created": "2024-09-03T19:42:04.877357Z",
                    "modified": "2024-09-03T19:42:04.877380Z",
                    "uuid": "fca9d15b-8a41-4353-a84e-5b9795f5f5a8",
                    "action_type": "notified",
                    "completed_at": null,
                    "error_reason": "email_error"
                },
                {
                    "created": "2024-09-03T19:42:04.898243Z",
                    "modified": "2024-09-03T19:42:04.898263Z",
                    "uuid": "525b77c0-9466-4faa-b729-055a8d73a40e",
                    "action_type": "learner_linked",
                    "completed_at": "2024-09-03T19:42:04.897991Z",
                    "error_reason": null
                }
            ],
            "earliest_possible_expiration": {
                "date": "2024-12-02T19:42:04Z",
                "reason": "NINETY_DAYS_PASSED"
            },
            "recent_action": {
                "action_type": "assigned",
                "timestamp": "2024-09-03T19:42:04Z"
            },
            "learner_state": "failed",
            "error_reason": {
                "action_type": "notified",
                "error_reason": "email_error"
            }
        }
    ],
    "learner_state_counts": [
        {
            "learner_state": "failed",
            "count": 1
        }
    ]
}

This leaves the admin-portal assignment table in a state where the assignment is visible to the admin with an indication of what has gone wrong and how to work around it:
image

Furthermore, since the assignment record's state is still allocated, the assigned course is visible and actionable to the learner from the learner portal MFE:
image

Merge checklist:

  • ./manage.py makemigrations has been run
    • Note: This must be run if you modified any models.
      • It may or may not make a migration depending on exactly what you modified, but it should still be run.

Post merge:

  • Ensure that your changes went out to the stage instance
  • Deploy to prod instance

@iloveagent57 iloveagent57 force-pushed the aed/no-email-failure branch 4 times, most recently from 4ec1a6e to dcaac35 Compare September 4, 2024 16:49
Q(state=LearnerContentAssignmentStateChoices.ALLOCATED) &
Q(has_errored_notification=True) &
Q(has_notification=False),
then=Value(AssignmentLearnerStates.FAILED),
Copy link
Contributor

@pwnage101 pwnage101 Sep 4, 2024

Choose a reason for hiding this comment

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

I think this should work (not creating a new learner_state specifically for this case) for keeping the assignment visible in the Assigned table, but I'm not 100% sure. The ticket description recommends a new learner state, but just wanted to make sure we confirmed the frontend doesn't actually need a new learner state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, the frontend already respects the FAILED value, that's how we get the "Failed: Bad Email" in the admin-portal screenshot in the description.


assert response.status_code == status.HTTP_200_OK
assert response.json().get('state') == LearnerContentAssignmentStateChoices.ALLOCATED
assert response.json().get('learner_state') == 'failed'
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: use constant instead of magic string.

Copy link
Contributor

@pwnage101 pwnage101 left a comment

Choose a reason for hiding this comment

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

the rest LGTM

When the assignment email notification task fails, the corresponding
assignment is now NOT moved to an errored state, but instead left
in an allocated state. Furthermore, the admin-serializer for the assignment
now indicates a "failed" _learner_ state, and an error reason related
to the failed notification attempt.
ENT-9037
@iloveagent57 iloveagent57 merged commit d4b359b into main Sep 4, 2024
3 checks passed
@iloveagent57 iloveagent57 deleted the aed/no-email-failure branch September 4, 2024 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants