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: Add override option to upgrade CTA utils and add course_run_key identifier #35441

Merged
merged 6 commits into from
Sep 16, 2024

Conversation

aht007
Copy link
Member

@aht007 aht007 commented Sep 6, 2024

Description

This PR introduces override capabilities to the EcommerceService class utilities. Currently, upgrade CTA URLs across edx-platform depend on the get_checkout_page_url and get_absolute_ecommerce_url methods, both of which are closely tied to the Oscar-based ecommerce system. This tight coupling presents challenges when attempting to integrate alternative ecommerce systems. With this PR, alternative implementations for these methods can be provided by leveraging the pluggable_override functionality from edx_django_utils.

Other information

  • Does this change depend on other changes elsewhere?
    To utilize this functionality, developers must define the path to the alternative implementations in the settings file using the override flags.
    Override flags being introduced in this PR:
  1. OVERRIDE_GET_CHECKOUT_PAGE_URL
  2. OVERRIDE_GET_ABSOLUTE_ECOMMERCE_URL

Override implementation needs to be housed somewhere else outside of edx-platform which can be a pluggable app installed independently in edx-platform requirements

@aht007 aht007 marked this pull request as draft September 6, 2024 10:43
@aht007 aht007 marked this pull request as ready for review September 10, 2024 07:24
@aht007 aht007 force-pushed the aht007/SONIC-645-CTAs-upgrade branch 3 times, most recently from 230a2cb to 37e2d65 Compare September 10, 2024 07:34
Copy link
Member

@NawfalAhmed NawfalAhmed left a comment

Choose a reason for hiding this comment

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

Nice work!

@aht007 aht007 changed the title feat: CTAs upgrade feat: Add override option to upgrade CTA utils and add course_run_key identifier Sep 10, 2024
@kdmccormick kdmccormick self-requested a review September 11, 2024 10:55
@kdmccormick kdmccormick removed their request for review September 12, 2024 11:28
@bmtcril bmtcril self-requested a review September 12, 2024 14:52
@bmtcril
Copy link
Contributor

bmtcril commented Sep 12, 2024

Hi @aht007 , I was working with @grmartin on this deprecation ticket: openedx/public-engineering#272 . This PR seems like an incremental step toward the goals in that ticket, which is great!

As mentioned in the ticket, the whole commerce app has been slated for deprecation for around 5 years. The goal is to find the places that explicitly call into commerce and make them more generic hooks / events/ filters so that any system (even non-commerce, such as a course recommendation engine) can extend them with more flexibility without breaking 2U's current design.

The hope was that we could add in those extension points as part of the deprecation to prevent you all from having to do all of this work now, then have to redo everything again when we deprecate commerce. By doing that work yourselves you would be best able to make sure the 2U specific functionality is preserved and tested as part of development, rather than needing to work with a contribution from the community that may not fit your needs.

To be clear, I don't have any problems with this PR as-is but want to make sure the direction is clear and hopefully save you all some work going forward.

@aht007
Copy link
Member Author

aht007 commented Sep 13, 2024

Hi @brian-smith-tcril,

Thank you for sharing your thoughts and reviewing the PR.
At the moment, our priorities and timelines don't allow us to take on this work, as it's a significant change that requires thorough coordination and communication, in addition to the technical aspects. That said, I’ll consult with the team and managers and follow up with you on this.

However, since the discussions around this could take some time, I would appreciate it if we could proceed with this PR for now.

@bmtcril
Copy link
Contributor

bmtcril commented Sep 13, 2024

@aht007 I think you pinged the wrong Brian, but yes I think you can consider that feedback non-blocking. It looks like you already have the required approvals so you should be all set.

@aht007
Copy link
Member Author

aht007 commented Sep 13, 2024

@bmtcril Ah sorry for the oversight and thank you.

@aht007 aht007 force-pushed the aht007/SONIC-645-CTAs-upgrade branch from b30f7e2 to 104ac81 Compare September 16, 2024 07:02
@aht007 aht007 merged commit 6a63cfc into master Sep 16, 2024
49 checks passed
@aht007 aht007 deleted the aht007/SONIC-645-CTAs-upgrade branch September 16, 2024 08:15
@edx-pipeline-bot
Copy link
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
Contributor

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

1 similar comment
@edx-pipeline-bot
Copy link
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.

7 participants