feat: convert CEUs to decimal #3217

asadali145 · 2024-11-04T11:17:56Z

What are the relevant tickets?

Closes https://github.com/mitodl/hq/issues/5834

Description (What does it do?)

Converts CEUs from string to decimal to avoid issues.

How can this be tested?

Checkout master branch and create the data
Create a few certificate pages for program and course pages. Add CEUs, Keep a few CEUs empty, and override a couple.
Create multiple course runs and program certificates.
Now checkout this branch and apply the migration
Test the following:
- CEUs are visible on the product page if the certificate.CEUs exist, but are not visible if only override exists without the certificate.CEUs
- CEUs are visible in the certificate preview if the certificate.CEUs, not visible if only override
- CEUs are visible in the certificates if CEUs exist or Override is visible when CEUs exist.
- Credits are correct in api/courses, should be null or decimal
- Credits are correct in api/programs, should be null or decimal for each course and null or decimal for the program itself
- CEUs are visible in the dashboard receipt.
- I am not sure about the usage of build_course_run_credential and build_program_credential, But we should verify that CEUs are returned in the data. I verified it through shell
Verify that nothing changes display-wise and no breaking changes.

arslanashraf7 · 2024-11-28T11:47:25Z

@asadali145 could you rebase this PR?

asadali145 · 2024-11-28T13:11:59Z

@asadali145 could you rebase this PR?

@arslanashraf7 Done!

arslanashraf7

Left a couple of changes.

arslanashraf7 · 2024-11-29T07:44:49Z

cms/models.py

+            "ceus": self.certificate_page.normalized_ceus
+            if self.certificate_page
+            else None,
        }


NIT: Keeping the same format of how we are using other properties, we might just want to move the if/else inside the normalized_ceus method.

normalized_ceus is a property of CertificatePage and this usage is in ProductPage. We cannot check this in normalized_ceus.

arslanashraf7 · 2024-11-29T10:01:38Z

cms/models.py

+        """
+        Normalizes the CEUs from decimal to string. Removes the trailing zeros if any.
+        """
+        return f"{self.CEUs.normalize():f}" if self.CEUs else None


Wouldn't a simple str() conversion be better?

str does not remove trailing zeros, This property is mostly used for display purposes, so, we don't need trailing zeros. That is why I used normalize.

I noted this because there are many courses in production that have .0 format at least. You can find them in /api/courses and search for credits. I don't know if the consumers of the APIs can break if they don't find .0s that's why I wanted to make sure that we keep the exact same format for the APIs to keep the external consumers from breaking.

Confirmed with Matt that this is OKay with MIT Learn. I will confirm with Bon today.

arslanashraf7 · 2024-11-29T10:29:19Z

cms/models.py

+        decimal_places=2,
+        max_digits=5,
+        help_text="Optional field for CEU (continuing education unit).",


This allows -ve values too. We will need to add validators.

Fixed in 85b1c00

arslanashraf7 · 2024-11-29T11:08:15Z

courses/sync_external_courses/emeritus_api.py

@@ -120,7 +121,7 @@ def __init__(self, emeritus_course_json):
        self.format = emeritus_course_json.get("format")
        self.category = emeritus_course_json.get("Category", None)
        self.image_name = emeritus_course_json.get("image_name", None)
-        self.CEUs = str(emeritus_course_json.get("ceu") or "")
+        self.CEUs = Decimal(str(emeritus_course_json.get("ceu"))) or None


This fails on null CEU values

Good Catch!

Fixed in 85b1c00

arslanashraf7

👍 Just some thoughts and one small change.

arslanashraf7 · 2024-12-02T12:04:34Z

cms/models.py

-        help_text="Optional text field for CEU (continuing education unit).",
+        decimal_places=2,
+        max_digits=5,
+        validators=[MinValueValidator(Decimal("0.01"))],


I think this should be zero otherwise it will never allow them to make CEUs Zero if needed in future revisions.

Fixed in f005f49

arslanashraf7

👍

review changes revert unintended change recreate migration

asadali145 · 2025-01-06T08:57:04Z

@arslanashraf7 This should be good to review.

odlbot temporarily deployed to xpro-ci-pr-3217 November 4, 2024 11:19 Inactive

odlbot temporarily deployed to xpro-ci-pr-3217 November 5, 2024 11:53 Inactive

odlbot temporarily deployed to xpro-ci-pr-3217 November 5, 2024 12:26 Inactive

odlbot temporarily deployed to xpro-ci-pr-3217 November 5, 2024 13:16 Inactive

odlbot temporarily deployed to xpro-ci-pr-3217 November 5, 2024 14:41 Inactive

odlbot deployed to xpro-ci-pr-3217 November 6, 2024 07:12 View deployment

asadali145 force-pushed the asad/convert-ceus-to-decimal branch 2 times, most recently from 638451b to 85b997f Compare November 14, 2024 11:40

asadali145 marked this pull request as ready for review November 14, 2024 12:04

asadali145 added the Needs Review label Nov 14, 2024

asadali145 requested a review from arslanashraf7 November 14, 2024 12:05

arslanashraf7 self-assigned this Nov 14, 2024

asadali145 force-pushed the asad/convert-ceus-to-decimal branch from 85b997f to ef3d652 Compare November 22, 2024 13:26

asadali145 force-pushed the asad/convert-ceus-to-decimal branch from ef3d652 to 5eabdca Compare November 28, 2024 13:11

arslanashraf7 requested changes Nov 29, 2024

View reviewed changes

asadali145 force-pushed the asad/convert-ceus-to-decimal branch from 4f7601c to 85b1c00 Compare December 2, 2024 08:02

asadali145 requested a review from arslanashraf7 December 2, 2024 08:03

arslanashraf7 approved these changes Dec 2, 2024

View reviewed changes

arslanashraf7 added Waiting on Author and removed Needs Review labels Dec 2, 2024

asadali145 added Needs Review and removed Waiting on Author labels Dec 3, 2024

asadali145 requested a review from arslanashraf7 December 3, 2024 10:23

arslanashraf7 approved these changes Dec 3, 2024

View reviewed changes

arslanashraf7 added Waiting on Author and removed Needs Review labels Dec 3, 2024

asadali145 force-pushed the asad/convert-ceus-to-decimal branch from f005f49 to 80d44eb Compare December 6, 2024 07:19

feat: convert CEUs to decimal

af1f082

review changes revert unintended change recreate migration

recreate migration

88ce0dd

asadali145 force-pushed the asad/convert-ceus-to-decimal branch from 80d44eb to 88ce0dd Compare January 3, 2025 11:17

asadali145 added 3 commits January 3, 2025 18:05

refactor: return CEUs as decimal

91df8c1

fix tests

72055a5

fix tests

0a95f34

asadali145 added Needs Review and removed Waiting on Author labels Jan 6, 2025

arslanashraf7 approved these changes Jan 6, 2025

View reviewed changes

arslanashraf7 added Waiting on Author and removed Needs Review labels Jan 6, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: convert CEUs to decimal #3217

feat: convert CEUs to decimal #3217

asadali145 commented Nov 4, 2024 •

edited

Loading

arslanashraf7 commented Nov 28, 2024

asadali145 commented Nov 28, 2024

arslanashraf7 left a comment

arslanashraf7 Nov 29, 2024

asadali145 Dec 2, 2024

arslanashraf7 Nov 29, 2024

asadali145 Dec 2, 2024

arslanashraf7 Dec 2, 2024 •

edited

Loading

asadali145 Dec 6, 2024

arslanashraf7 Nov 29, 2024

asadali145 Dec 2, 2024

arslanashraf7 Nov 29, 2024

asadali145 Dec 2, 2024

asadali145 Dec 2, 2024

arslanashraf7 left a comment

arslanashraf7 Dec 2, 2024

asadali145 Dec 2, 2024

arslanashraf7 left a comment

asadali145 commented Jan 6, 2025

feat: convert CEUs to decimal #3217

Are you sure you want to change the base?

feat: convert CEUs to decimal #3217

Conversation

asadali145 commented Nov 4, 2024 • edited Loading

What are the relevant tickets?

Description (What does it do?)

How can this be tested?

arslanashraf7 commented Nov 28, 2024

asadali145 commented Nov 28, 2024

arslanashraf7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arslanashraf7 Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arslanashraf7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arslanashraf7 left a comment

Choose a reason for hiding this comment

asadali145 commented Jan 6, 2025

asadali145 commented Nov 4, 2024 •

edited

Loading

arslanashraf7 Dec 2, 2024 •

edited

Loading