feat: add Accredible integration #2675

kyrylo-kh · 2025-01-02T10:46:22Z

Description:
Add integration with Accredible service for badges

openedx-webhooks · 2025-01-02T10:46:26Z

Thanks for the pull request, @kyrylo-kh!

This repository is currently maintained by @2U-aperture.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

deborahgu

Non-blocking concern: It seems sensible to have verifiable credentials as a django app in the IDA, but I would expect it to be generic, with the Accredible integration as a plugin (which could be provided in an openedx plugin repo). Otherwise it seems like we're recreating the same technical debt issues that we've been removing for the last couple of years, putting a specific 3rd party SaaS provider's code, and a specific implementation, directly into the IDA.

Especially since we have a good plug-in framework now, I would think this would be an opportunity for a formal openedx plugin (which would also be modeling a great practice for the community).

deborahgu · 2025-01-21T15:43:18Z

Non-blocking concern

The more I think about it, the more I feel like this is a blocking concern. Certainly on the edx-platform side, there's an acknowledgement that built in optional integrations to braze, hubspot, salesforce, etc. are grandfathered in for now but will inevitably be formally deprecated, and new third-party APIs should be written as plug-ins.

Unless there's a reason we can't, it seems to me that moving forward, all new integrations to a specific third-party vendor should be in a plugin. In this case, again, in a public, official, openedx plugin repo, and the code here should be entirely extensible generics. Is there a reason we can't or shouldn't do that?

(FWIW I felt the same way about Credly but didn't say anything then; since that time it's become more clear that Axim views vendor-specific third party API integrations as plugin material, so consistency would suggest following this model.)

e0d · 2025-01-22T14:14:05Z

@deborahgu Does the Credentials IDA support plugins using the event driving model, or at all today?

deborahgu · 2025-01-22T18:33:51Z

@deborahgu Does the Credentials IDA support plugins using the event driving model, or at all today?

I'm not positive. Certainly I know we can run our standard edx_django_utils plug-in utilities. And I can see that openedx_events is already used in the badges implementation on credentials, so I'd be surprised if we could not use the hooks and filters implementation.

The OEP for hooks specifically references Accredible as its very first use case, and all of the existing filter documentation on design practices and how-tos use "third-party certificate verification service" as their use case. (obviously those examples all take place on edx-platform, where the course certificates are currently generated.) But that reference implantation is only a couple of years ago, so I assume that credentials was part of the infrastructural plan around it.

kyrylo-kh · 2025-01-27T09:47:32Z

@deborahgu Answer for your concern

The proposal did not mention moving the customization for Credly into a separate plugin; therefore, the new integration with Accredible was added alongside the existing one. This approach allows for reusing existing functionality and avoids duplicating the same code (such as the badge issuance mechanism, base classes for badges, configurations, issuers, etc.).
All the new logic related to badges is located in its own separate app at credentials/apps/badges, and changes to the main codebase (excluding translations and documentation) outside of it are minimal. Therefore, in my opinion, it makes sense to keep everything in one place rather than splitting it between this location and a separate plugin.
We created a separate plugin for quick and convenient badge configuration, specifically designed for the current solution.

This implementation was agreed with the @e0d

e0d · 2025-02-06T14:24:16Z

@GlugovGrGlib will you be able to find time to review? I believe you did some thinking about adding plugins to the Credentials IDA, but I'm not sure if there was any progress on that front.

e0d · 2025-02-06T14:26:53Z

@deborahgu I agree with your overall argument here. However, I'd like to propose that we capture the work as technical debt rather than block this change on the refactoring. I'll do my best to prioritize getting in done in the next ~2 quarters or so. @cmltaWt0 I believe you have done a back of the envelope estimate? Could that be the basis of a ticket to capture the required work here?

deborahgu · 2025-02-06T21:18:30Z

@e0d I'm amenable to looking at this work with the plan to switch to a plugin based-system. I will go ahead and review this code now on its own merits, and I will look forward to seeing the new scope of work. (cc @justinhynes @kbuchanan-2u)

deborahgu

this looks good to me! I'm approving, although I did put in some nonblocking feedback.

deborahgu · 2025-02-06T21:38:06Z

credentials/apps/badges/accredible/api_client.py

+            site_id (int): ID of the site.
+
+        Returns:
+            int | None: processed items.


[two issues, both nonblocking]

would you be able to rewrite this docstring for clarity? I was scratching my head for a while trying to figure out why on earth you were returning a length, because I thought that the return value here was supposed to actually be something about the processed items. It took me a while to realize that it meant something more like

Suggested change

int | None: processed items.

int: Number of groups synchronized

Also, in general, I feel like the fall through behavior should just be returning 0 if raw_groups is empty. On the one hand, it's not that difference in terms of consumption. if None and if 0 will have the same effect. But on the other hand, it's good to have a method that always returns a single type (and in fact, you've type hinted this as -> int, not as -> Optional[int]).

deborahgu · 2025-02-06T21:47:14Z

credentials/apps/badges/accredible/utils.py

+    if accredible_config.get("USE_SANDBOX"):
+        return accredible_config["ACCREDIBLE_SANDBOX_API_BASE_URL"]
+
+    return accredible_config["ACCREDIBLE_API_BASE_URL"]


this one seems like it actually should get a real test.

deborahgu · 2025-02-06T21:56:51Z

credentials/apps/badges/management/commands/sync_accredible_groups.py

+        api_config_id = options.get("api_config_id")
+
+        if site_id is None:
+            self.stdout.write(f"Side ID wasn't provided: assuming site_id = {DEFAULT_SITE_ID}")


Suggested change

self.stdout.write(f"Side ID wasn't provided: assuming site_id = {DEFAULT_SITE_ID}")

self.stdout.write(f"Site ID wasn't provided: assuming site_id = {DEFAULT_SITE_ID}")

deborahgu · 2025-02-06T22:01:22Z

credentials/apps/badges/signals/handlers.py

+    if origin == CredlyBadgeTemplate.ORIGIN:
+        CredlyBadgeTemplateIssuer().award(username=username, credential_id=badge_template_id)
+    elif origin == AccredibleGroup.ORIGIN:
+        AccredibleBadgeTemplateIssuer().award(username=username, credential_id=badge_template_id)


seems like it's worth a test

deborahgu · 2025-02-06T22:01:34Z

credentials/apps/badges/signals/handlers.py

+    if origin == CredlyBadgeTemplate.ORIGIN:
+        CredlyBadgeTemplateIssuer().revoke(badge_template_id, username)
+    elif origin == AccredibleGroup.ORIGIN:
+        AccredibleBadgeTemplateIssuer().revoke(badge_template_id, username)


kyrylo-kh requested a review from a team as a code owner January 2, 2025 10:46

kyrylo-kh marked this pull request as draft January 2, 2025 10:46

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jan 2, 2025

kyrylo-kh force-pushed the feat/accredible-integration branch 5 times, most recently from 11374b7 to 0a7f487 Compare January 9, 2025 10:43

kyrylo-kh force-pushed the feat/accredible-integration branch 3 times, most recently from 207ae36 to 2c042b4 Compare January 14, 2025 10:01

feat: add Accredible integration

9855ff4

kyrylo-kh force-pushed the feat/accredible-integration branch from 2c042b4 to 9855ff4 Compare January 15, 2025 09:35

kyrylo-kh marked this pull request as ready for review January 15, 2025 10:03

deborahgu reviewed Jan 21, 2025

View reviewed changes

GlugovGrGlib self-requested a review January 23, 2025 11:31

deborahgu approved these changes Feb 6, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add Accredible integration #2675

feat: add Accredible integration #2675

kyrylo-kh commented Jan 2, 2025

openedx-webhooks commented Jan 2, 2025 •

edited

Loading

🔘 Get product approval

🔘 Provide context

🔘 Get a green build

Where can I find more information?

When can I expect my changes to be merged?

deborahgu left a comment

deborahgu commented Jan 21, 2025 •

edited

Loading

e0d commented Jan 22, 2025

deborahgu commented Jan 22, 2025

kyrylo-kh commented Jan 27, 2025

e0d commented Feb 6, 2025

e0d commented Feb 6, 2025

deborahgu commented Feb 6, 2025

deborahgu left a comment

deborahgu Feb 6, 2025

deborahgu Feb 6, 2025

deborahgu Feb 6, 2025

deborahgu Feb 6, 2025

deborahgu Feb 6, 2025

	int \| None: processed items.
	int: Number of groups synchronized

	self.stdout.write(f"Side ID wasn't provided: assuming site_id = {DEFAULT_SITE_ID}")
	self.stdout.write(f"Site ID wasn't provided: assuming site_id = {DEFAULT_SITE_ID}")

feat: add Accredible integration #2675

Are you sure you want to change the base?

feat: add Accredible integration #2675

Conversation

kyrylo-kh commented Jan 2, 2025

openedx-webhooks commented Jan 2, 2025 • edited Loading

🔘 Get product approval

🔘 Provide context

🔘 Get a green build

Where can I find more information?

When can I expect my changes to be merged?

deborahgu left a comment

Choose a reason for hiding this comment

deborahgu commented Jan 21, 2025 • edited Loading

e0d commented Jan 22, 2025

deborahgu commented Jan 22, 2025

kyrylo-kh commented Jan 27, 2025

e0d commented Feb 6, 2025

e0d commented Feb 6, 2025

deborahgu commented Feb 6, 2025

deborahgu left a comment

Choose a reason for hiding this comment

deborahgu Feb 6, 2025

Choose a reason for hiding this comment

deborahgu Feb 6, 2025

Choose a reason for hiding this comment

deborahgu Feb 6, 2025

Choose a reason for hiding this comment

deborahgu Feb 6, 2025

Choose a reason for hiding this comment

deborahgu Feb 6, 2025

Choose a reason for hiding this comment

openedx-webhooks commented Jan 2, 2025 •

edited

Loading

deborahgu commented Jan 21, 2025 •

edited

Loading