feat: adds attribute to hide course prices when zero #1788

tecoholic · 2023-07-04T13:11:16Z

Description

This adds a new attribute hide_course_price_when_zero to the EnterpriseCustomer model, which will hide the pricing information from the enrollment page when the final price of a premium course mode is Zero.

Testing instructions

Setup a working devstack setup for edx-enterprise. Required services: LMS, ECommerce, Discovery and Enterprise Catalog (this needs to be setup separately)
Check out the PR branch
OPTIONAL: Install edx-enterprise in your LMS container if not already installed.
```
# From devstack dir
make dev.shell.lms
pip install -e /edx/src/edx-enterprise
```

Apply the migrations

# From devstack dir
make dev.shell.lms
./manage.py lms migrate

Go to enterprise customer admin page copy the UUID of the test enterprise
Go to enterprise catalog admin page and copy the UUID of the catalog attached to test enterprise
Go to discovery search page and copy the key of one of the courses included in the enterprise catalog. If you are using the "All course runs" catalog used in the devstack setup, then it can be any course's key which has a paid seat.
Visit the URL http://localhost:18000/enterprise/<enterprise_uuid>/course/<course_key>/enroll/, eg., http://localhost:18000/enterprise/753e5f69-ef92-42ab-ba07-9326d76603e8/course/edX%2BP315/enroll/. This should show the course details along with the price of the course.
Go to the Ecommerce /courses/ page and add an Enterprise Offer using the enterprise and catalog UUIDs and set the discount percent to 100% (this will mark the final price to zero). Note: Use 006NFJHRVSD683NDI8 for the Salesforce opportunity ID for testing.
Now refresh the enrollment page, it should show the final price as zero.
Go to the enterprise customer admin page and edit the test enterprise. Check the Hide course price when zero flag and save the customer.
Refresh the enrollment page. The pricing information would have disappeared.

Merge checklist

Any new requirements are in the right place (do not manually modify the requirements/*.txt files)
- base.in if needed in production but edx-platform doesn't install it
- test-master.in if edx-platform pins it, with a matching version
- make upgrade && make requirements have been run to regenerate requirements
make static has been run to update webpack bundling if any static content was updated
./manage.py makemigrations has been run
- Checkout the Database Migration Confluence page for helpful tips on creating migrations.
- 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.
- This should be run from either a venv with all the lms/edx-enterprise requirements installed or if you checked out edx-enterprise into the src directory used by lms, you can run this command through an lms shell.
  - It would be ./manage.py lms makemigrations in the shell.
Version bumped
Changelog record added
Translations updated (see docs/internationalization.rst but also this isn't blocking for merge atm)

Post merge

Tag pushed and a new version released
- Note: Assets will be added automatically. You just need to provide a tag (should match your version number) and title and description.
After versioned build finishes in GitHub Actions, verify version has been pushed to PyPI
- Each step in the release build has a condition flag that checks if the rest of the steps are done and if so will deploy to PyPi.
  (so basically once your build finishes, after maybe a minute you should see the new version in PyPi automatically (on refresh))
PR created in edx-platform to upgrade dependencies (including edx-enterprise)
- This must be done after the version is visible in PyPi as make upgrade in edx-platform will look for the latest version in PyPi.
- Note: the edx-enterprise constraint in edx-platform must also be bumped to the latest version in PyPi.

openedx-webhooks · 2023-07-04T13:11:21Z

Thanks for the pull request, @tecoholic!

What's next?

Please work through the following steps to get your changes 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.

🔘 Let us know that your PR is ready for review:

Who will review my changes?

This repository is currently unmaintained.

To get help with finding a technical reviewer, tag the community contributions project manager for this PR in a comment and let them know that your changes are ready for review:

On the right-hand side of the PR, find the Contributions project, click the caret in the top right corner to expand it, and check the "Primary PM" field for the name of your PM.
Find their GitHub handle here.

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.

Agrendalath

@tecoholic, feel free to squash the commits. Also, should we bump the version and update the changelog? Or will we do this after the upstream review to avoid frequent conflicts?

👍

I tested this: checked that it's possible to hide prices
I read through the code
I checked for accessibility issues: n/a
Includes documentation: n/a
I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository: n/a

Agrendalath · 2023-07-07T13:58:38Z

enterprise/utils.py

+        except ValueError:
+            LOGGER.warning(
+                'hide_price_when_zero: Could not convert price of course mode "%s" to int.',
+                mode['title']


We could also log the final_price here to understand why this happened.

Agrendalath · 2023-07-07T15:15:38Z

enterprise/utils.py

+        try:
+            numbers = re.findall(r'\d+', mode['final_price'])
+            mode['hide_price'] = int(''.join(numbers)) == 0
+        except ValueError:


This can happen only when mode['original_price'] does not contain digits (mode['final_price'] is generated in get_course_final_price).
Technically, it could also happen when the mode['min_price'] does not contain digits, but this should raise an exception in the format_price function.

Is it even possible? If yes, what does it mean? Is the course free? We can keep this exception handling; I'm just curious.

@Agrendalath To be fair, I didn't quite find an edge case while testing for this. The final final_price is a formatted string from format_price which is bound to either provide a properly formatted string, or throw an exception.

But for some reason, I wasn't comfortable with parsing string using regex with no safeguards, so added the try block.

mphilbrick211 · 2023-07-19T17:24:13Z

Hi @Agrendalath! Are you able to re-run the checks here?

Agrendalath · 2023-07-19T17:29:04Z

@mphilbrick211, done.

Agrendalath · 2023-07-19T17:53:24Z

@tecoholic, the failing TestEnterpriseUtils.test_get_all_field_names_1 test seems to be related to this PR. It's likely the order in the tests/test_utilities.py file.

tecoholic · 2023-07-21T15:05:31Z

@Agrendalath I have fixed the test and updated the PR.

mphilbrick211 · 2023-07-24T19:18:39Z

@Agrendalath - the tests need to be enabled again, if you wouldn't mind :)

Agrendalath · 2023-07-31T10:28:06Z

@mphilbrick211, the tests were already green when I saw your comment, but I rebased the branch just in case.

Side note: I don't have permission to approve the CI in this repository, but the tests are running correctly when I rebase the branch.

mphilbrick211 · 2023-08-02T12:47:06Z

@Agrendalath - thanks! is this now ready for review?

Agrendalath · 2023-08-02T12:55:14Z

@mphilbrick211, yes. We will update the changelog and version after the review to avoid merge conflicts.

mphilbrick211 · 2023-08-02T12:56:21Z

Hi @openedx/enterprise-titans! Would someone be able to please review this for us? Thanks!

mphilbrick211 · 2023-08-07T18:13:44Z

@Agrendalath - hi there! Would you mind enabling tests again?

Agrendalath · 2023-08-08T08:58:30Z

@tecoholic, it looks like we need to change the migration prefix from 0175 to 0178.

mphilbrick211 · 2023-09-12T14:32:49Z

Hi @tecoholic! Some branch conflicts have popped up. Would you mind taking a look?

mphilbrick211 · 2023-10-03T02:24:52Z

Hi @openedx/enterprise-titans! Is someone able to help review/merge this PR? If so, could you provide an ETA on when you'd be able to do so?

mphilbrick211 · 2023-10-24T20:32:24Z

Hi @openedx/enterprise-titans - just following up to see when it will be possible to get this reviewed? Once you let us know, we'll address the failing tests.

This adds a new attribute `hide_course_price_when_zero` to the EnterpriseCustomer model, which will hide the pricing information from the enrollment page when the final price of a premium course mode is Zero.

tecoholic · 2023-12-08T10:22:32Z

@openedx/enterprise-titans This PR has been rebased on master and DB migration has been updated to the latest number. Kindly review.

cc: @e0d

mphilbrick211 · 2024-02-21T14:52:08Z

Hi @openedx/2u-titans! Are you still still reviewing pull requests? If so, could you please review / merge this for us? Thank you!

tecoholic · 2024-08-12T04:36:24Z

Closing this as we don't use the ecommerce based flow anymore and won't be pursuing upstreaming of these changes.

cc: @Agrendalath

openedx-webhooks · 2024-08-12T04:36:29Z

@tecoholic Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jul 4, 2023

tecoholic marked this pull request as ready for review July 4, 2023 13:35

tecoholic mentioned this pull request Jul 6, 2023

feat: add a flag to hide course prices in enrollment page when zero open-craft/edx-enterprise#6

Merged

tecoholic force-pushed the tecoholic/BB-7541-hide-price-in-enrollment-page branch from b9eb7c8 to 62189af Compare July 6, 2023 11:16

Agrendalath approved these changes Jul 7, 2023

View reviewed changes

tecoholic force-pushed the tecoholic/BB-7541-hide-price-in-enrollment-page branch from 62189af to ed04eed Compare July 14, 2023 06:21

mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Jul 19, 2023

Agrendalath force-pushed the tecoholic/BB-7541-hide-price-in-enrollment-page branch from ed04eed to fd9e9e9 Compare July 19, 2023 17:28

tecoholic force-pushed the tecoholic/BB-7541-hide-price-in-enrollment-page branch from fd9e9e9 to 49c2823 Compare July 21, 2023 15:05

e0d removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Jul 28, 2023

Agrendalath force-pushed the tecoholic/BB-7541-hide-price-in-enrollment-page branch from 49c2823 to eb73871 Compare July 31, 2023 09:51

mphilbrick211 requested a review from a team August 2, 2023 12:46

tecoholic force-pushed the tecoholic/BB-7541-hide-price-in-enrollment-page branch from eb73871 to 3a0c531 Compare August 7, 2023 17:37

Agrendalath force-pushed the tecoholic/BB-7541-hide-price-in-enrollment-page branch from 3a0c531 to 4320731 Compare August 8, 2023 08:52

mphilbrick211 added the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label Oct 24, 2023

tecoholic force-pushed the tecoholic/BB-7541-hide-price-in-enrollment-page branch from 308d3ac to 168f613 Compare November 10, 2023 06:07

mphilbrick211 removed needs test run Author's first PR to this repository, awaiting test authorization from Axim waiting for eng review PR is ready for review. Review and merge it, or suggest changes. labels Nov 13, 2023

tecoholic force-pushed the tecoholic/BB-7541-hide-price-in-enrollment-page branch from 168f613 to bf44e9a Compare November 14, 2023 13:10

tecoholic and others added 7 commits December 8, 2023 11:01

feat: adds attribute to hide course prices when zero

7740dcd

This adds a new attribute `hide_course_price_when_zero` to the EnterpriseCustomer model, which will hide the pricing information from the enrollment page when the final price of a premium course mode is Zero.

fix: removed unused variable from template context

4f37ab1

fix: quality issue and add the new attribute to test

5f2f7fb

fix: update the help text in the db migration

f8caa06

feat: adds final price to logging

9a650e2

fix: change the attribute in the utils test causing test failure

8413dfe

refactor: renamed the migration file

d4b04a3

tecoholic force-pushed the tecoholic/BB-7541-hide-price-in-enrollment-page branch from bf44e9a to 2dfbafd Compare December 8, 2023 05:41

chore: rebase on master and update db migration

f4a10ca

tecoholic force-pushed the tecoholic/BB-7541-hide-price-in-enrollment-page branch 2 times, most recently from 678cd9a to 1abdabe Compare December 8, 2023 05:54

fix: remove empty lines causing linting failure

d4ac14d

tecoholic force-pushed the tecoholic/BB-7541-hide-price-in-enrollment-page branch from 1abdabe to d4ac14d Compare December 8, 2023 06:03

fix: remove the empty line in tests file

219e3f1

mphilbrick211 added the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label Dec 11, 2023

0x29a mentioned this pull request Dec 30, 2023

feat: cherry-pick edx-enterprise customizations to Quince [BB-8368] open-craft/edx-enterprise#13

Merged

mphilbrick211 removed the request for review from a team July 30, 2024 21:35

mphilbrick211 added needs reviewer assigned PR needs to be (re-)assigned a new reviewer and removed waiting for eng review PR is ready for review. Review and merge it, or suggest changes. labels Jul 30, 2024

tecoholic closed this Aug 12, 2024

tecoholic mentioned this pull request Aug 23, 2024

feat: allow enrolling learners on invitation only courses open-craft/edx-enterprise#20

Merged

itsjeyd removed the needs reviewer assigned PR needs to be (re-)assigned a new reviewer label Sep 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: adds attribute to hide course prices when zero #1788

feat: adds attribute to hide course prices when zero #1788

tecoholic commented Jul 4, 2023 •

edited

Loading

openedx-webhooks commented Jul 4, 2023 •

edited

Loading

Agrendalath left a comment

Agrendalath Jul 7, 2023

Agrendalath Jul 7, 2023

tecoholic Jul 14, 2023 •

edited

Loading

mphilbrick211 commented Jul 19, 2023

Agrendalath commented Jul 19, 2023

Agrendalath commented Jul 19, 2023

tecoholic commented Jul 21, 2023

mphilbrick211 commented Jul 24, 2023

Agrendalath commented Jul 31, 2023

mphilbrick211 commented Aug 2, 2023

Agrendalath commented Aug 2, 2023

mphilbrick211 commented Aug 2, 2023

mphilbrick211 commented Aug 7, 2023

Agrendalath commented Aug 8, 2023

mphilbrick211 commented Sep 12, 2023

mphilbrick211 commented Oct 3, 2023

mphilbrick211 commented Oct 24, 2023

tecoholic commented Dec 8, 2023

mphilbrick211 commented Feb 21, 2024

tecoholic commented Aug 12, 2024

openedx-webhooks commented Aug 12, 2024

feat: adds attribute to hide course prices when zero #1788

feat: adds attribute to hide course prices when zero #1788

Conversation

tecoholic commented Jul 4, 2023 • edited Loading

Description

Testing instructions

Merge checklist

Post merge

openedx-webhooks commented Jul 4, 2023 • edited Loading

What's next?

🔘 Get product approval

🔘 Provide context

🔘 Get a green build

🔘 Let us know that your PR is ready for review:

Who will review my changes?

Where can I find more information?

When can I expect my changes to be merged?

Agrendalath left a comment

Choose a reason for hiding this comment

Agrendalath Jul 7, 2023

Choose a reason for hiding this comment

Agrendalath Jul 7, 2023

Choose a reason for hiding this comment

tecoholic Jul 14, 2023 • edited Loading

Choose a reason for hiding this comment

mphilbrick211 commented Jul 19, 2023

Agrendalath commented Jul 19, 2023

Agrendalath commented Jul 19, 2023

tecoholic commented Jul 21, 2023

mphilbrick211 commented Jul 24, 2023

Agrendalath commented Jul 31, 2023

mphilbrick211 commented Aug 2, 2023

Agrendalath commented Aug 2, 2023

mphilbrick211 commented Aug 2, 2023

mphilbrick211 commented Aug 7, 2023

Agrendalath commented Aug 8, 2023

mphilbrick211 commented Sep 12, 2023

mphilbrick211 commented Oct 3, 2023

mphilbrick211 commented Oct 24, 2023

tecoholic commented Dec 8, 2023

mphilbrick211 commented Feb 21, 2024

tecoholic commented Aug 12, 2024

openedx-webhooks commented Aug 12, 2024

tecoholic commented Jul 4, 2023 •

edited

Loading

openedx-webhooks commented Jul 4, 2023 •

edited

Loading

tecoholic Jul 14, 2023 •

edited

Loading