Skip to content

feat: receiver for invalidate certificate - #33319

Merged
ilee2u merged 5 commits into
masterfrom
ilee2u/invalidate-certificate-consumer
Oct 5, 2023
Merged

feat: receiver for invalidate certificate#33319
ilee2u merged 5 commits into
masterfrom
ilee2u/invalidate-certificate-consumer

Conversation

@ilee2u

@ilee2u ilee2u commented Sep 21, 2023

Copy link
Copy Markdown
Contributor
  • consumes event of exam attempt rejected
  • initial commit, need to make tests

Description

Describe what this pull request changes, and why. Include implications for people using this change.
Design decisions and their rationales should be documented in the repo (docstring / ADR), per
OEP-19, and can be
linked here.

Useful information to include:

  • Which edX user roles will this change impact? Common user roles are "Learner", "Course Author",
    "Developer", and "Operator".
  • Include screenshots for changes to the UI (ideally, both "before" and "after" screenshots, if applicable).
  • Provide links to the description of corresponding configuration changes. Remember to correctly annotate these
    changes.

Supporting information

Link to other information about the change, such as Jira issues, GitHub issues, or Discourse discussions.
Be sure to check they are publicly readable, or if not, repeat the information here.

Testing instructions

Please provide detailed step-by-step instructions for testing this change.

Deadline

"None" if there's no rush, or provide a specific date or event (and reason) if there is one.

Other information

Include anything else that will help reviewers and consumers understand the change.

  • Does this change depend on other changes elsewhere?
  • Any special concerns or limitations? For example: deprecations, migrations, security, or accessibility.
  • If your database migration can't be rolled back easily.

@ilee2u
ilee2u force-pushed the ilee2u/invalidate-certificate-consumer branch 3 times, most recently from 9b7185c to 0715dd1 Compare September 28, 2023 17:39
@ilee2u
ilee2u marked this pull request as ready for review September 28, 2023 17:39
@ilee2u
ilee2u changed the base branch from master to MST-1789-downstream-triggers September 29, 2023 15:28
@ilee2u
ilee2u force-pushed the ilee2u/invalidate-certificate-consumer branch 3 times, most recently from 5682ef2 to a414af3 Compare October 3, 2023 14:48
@ilee2u
ilee2u changed the base branch from MST-1789-downstream-triggers to master October 3, 2023 14:49
@ilee2u
ilee2u force-pushed the ilee2u/invalidate-certificate-consumer branch 2 times, most recently from b1e1b93 to d7f2e6d Compare October 3, 2023 17:32
Comment thread lms/djangoapps/certificates/api.py Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this appears to just invalidate a cert and not do anything exam specific. I think we can leave the exam context out of here and just call it invalidate_certificate. It might be used for other purposes in the future.

Comment thread lms/djangoapps/certificates/api.py Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think source might be a new parameter to this function. This is now called from multiple sources one is still certificate_service and the other is new (maybe exam_event?)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah good point, I missed that. Will add that as a parameter to this function.

@ilee2u
ilee2u force-pushed the ilee2u/invalidate-certificate-consumer branch 3 times, most recently from 2bb6625 to bff127d Compare October 5, 2023 16:57

@varshamenon4 varshamenon4 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks great! I really like the descriptive comments, particularly the clarifications around the legacy vs. current exams backend. Just one question in the comments. :)

Comment thread lms/djangoapps/certificates/api.py Outdated
Comment thread lms/djangoapps/certificates/services.py Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Really descriptive and helpful comments throughout! Nice. :)

@varshamenon4 varshamenon4 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm!

@ilee2u
ilee2u dismissed zacharis278’s stale review October 5, 2023 19:13

Already made the requested changes

ilee2u added 5 commits October 5, 2023 15:13
- consumes event of exam attempt rejected
- initial commit, need to make tests
- Still need to make this work
- Need to make tests work too
- tests still need to be tweaked
@ilee2u
ilee2u force-pushed the ilee2u/invalidate-certificate-consumer branch from bff127d to b059701 Compare October 5, 2023 19:13
@ilee2u
ilee2u enabled auto-merge (squash) October 5, 2023 19:13
@ilee2u
ilee2u merged commit 2ae0738 into master Oct 5, 2023
@ilee2u
ilee2u deleted the ilee2u/invalidate-certificate-consumer branch October 5, 2023 19:39
@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

1 similar comment
@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

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.

4 participants