From d3f65a9ff2ea44a93bdf64701b1474ca7137f7a0 Mon Sep 17 00:00:00 2001 From: Arunmozhi Date: Tue, 24 Sep 2024 14:39:31 +1000 Subject: [PATCH 1/5] chore: bump upload-artifact GH task to v4 --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9fc84af3fd..d231546e61 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -44,7 +44,7 @@ jobs: continue-on-error: ${{ matrix.status == 'ignored' }} - name: Upload coverage if: matrix.db-version == 'mysql80' - uses: actions/upload-artifact@v2 + uses: actions/upload-artifact@v4 with: name: coverage${{ matrix.pytest-split-group }} path: .coverage From c472007fa3ffa10b3949ec2103aef69170a07a0c Mon Sep 17 00:00:00 2001 From: Arunmozhi Date: Tue, 24 Sep 2024 15:12:42 +1000 Subject: [PATCH 2/5] fix: fallback to 'price' in ecommerce api loader The Django Oscar upgrade of ecommerce changed the item's price field from `price_excl_tax` to just `price` causing the EcommerceApi data loader to fail. This commit checks for the `price_excl_tax` key and falls back to the 'price' value. Ref: https://github.com/openedx/ecommerce/pull/4050 --- course_discovery/apps/course_metadata/data_loaders/api.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/course_discovery/apps/course_metadata/data_loaders/api.py b/course_discovery/apps/course_metadata/data_loaders/api.py index a63aca75e4..e0cfef8cf4 100644 --- a/course_discovery/apps/course_metadata/data_loaders/api.py +++ b/course_discovery/apps/course_metadata/data_loaders/api.py @@ -572,7 +572,10 @@ def update_seats(self, body): def update_seat(self, course_run, product_body): stock_record = product_body['stockrecords'][0] currency_code = stock_record['price_currency'] - price = Decimal(stock_record['price_excl_tax']) + if 'price_excl_tax' in stock_record: + price = Decimal(stock_record['price_excl_tax']) + else: + price = Decimal(stock_record['price']) sku = stock_record['partner_sku'] # For more context see ADR docs/decisions/0025-dont-sync-mobile-skus-on-discovery.rst From 198c47a20e1af6a101eb246ee8a5f1c2a01325a3 Mon Sep 17 00:00:00 2001 From: Arunmozhi Date: Wed, 25 Sep 2024 10:31:35 +1000 Subject: [PATCH 3/5] refactor: apply review suggestion on fix --- course_discovery/apps/course_metadata/data_loaders/api.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/course_discovery/apps/course_metadata/data_loaders/api.py b/course_discovery/apps/course_metadata/data_loaders/api.py index e0cfef8cf4..a8a6045227 100644 --- a/course_discovery/apps/course_metadata/data_loaders/api.py +++ b/course_discovery/apps/course_metadata/data_loaders/api.py @@ -572,10 +572,7 @@ def update_seats(self, body): def update_seat(self, course_run, product_body): stock_record = product_body['stockrecords'][0] currency_code = stock_record['price_currency'] - if 'price_excl_tax' in stock_record: - price = Decimal(stock_record['price_excl_tax']) - else: - price = Decimal(stock_record['price']) + price = Decimal(stock_record.get('price_excl_tax', stock_record['price'])) sku = stock_record['partner_sku'] # For more context see ADR docs/decisions/0025-dont-sync-mobile-skus-on-discovery.rst From 89ed83053c7523f044cbd401ccf71ed25fd2450d Mon Sep 17 00:00:00 2001 From: Arunmozhi Date: Thu, 26 Sep 2024 10:07:23 +1000 Subject: [PATCH 4/5] fix: update all references of `price_excl_tax` to price --- .../apps/course_metadata/data_loaders/api.py | 10 +++-- .../data_loaders/tests/mock_data.py | 39 +++++++++++++------ .../data_loaders/tests/test_api.py | 6 +-- 3 files changed, 37 insertions(+), 18 deletions(-) diff --git a/course_discovery/apps/course_metadata/data_loaders/api.py b/course_discovery/apps/course_metadata/data_loaders/api.py index a8a6045227..da66e72f03 100644 --- a/course_discovery/apps/course_metadata/data_loaders/api.py +++ b/course_discovery/apps/course_metadata/data_loaders/api.py @@ -643,9 +643,11 @@ def update_seat(self, course_run, product_body): def validate_stockrecord(self, stockrecords, title, product_class): """ Argument: - body (dict): product data from ecommerce, either entitlement or enrollment code + sockrecords (list): a list of stock records to validate from ecommerce + title (str): product title + product_class (str): either entitlement or enrollment code Returns: - product sku if no exceptions, else None + True when all validation checks pass, else None """ # Map product_class keys with how they should be displayed in the exception messages. product_classes = { @@ -680,7 +682,7 @@ def validate_stockrecord(self, stockrecords, title, product_class): try: currency_code = stock_record['price_currency'] - Decimal(stock_record['price_excl_tax']) + Decimal(stock_record.get('price_excl_tax', stock_record['price'])) sku = stock_record['partner_sku'] except (KeyError, ValueError): msg = 'A necessary stockrecord field is missing or incorrectly set for {product} {title}'.format( @@ -719,7 +721,7 @@ def update_entitlement(self, body): stock_record = stockrecords[0] currency_code = stock_record['price_currency'] - price = Decimal(stock_record['price_excl_tax']) + price = Decimal(stock_record.get('price_excl_tax', stock_record['price'])) sku = stock_record['partner_sku'] try: diff --git a/course_discovery/apps/course_metadata/data_loaders/tests/mock_data.py b/course_discovery/apps/course_metadata/data_loaders/tests/mock_data.py index 113dacee52..960440e2b2 100644 --- a/course_discovery/apps/course_metadata/data_loaders/tests/mock_data.py +++ b/course_discovery/apps/course_metadata/data_loaders/tests/mock_data.py @@ -195,7 +195,7 @@ "stockrecords": [ { "price_currency": "USD", - "price_excl_tax": "0.00", + "price": "0.00", "partner_sku": "sku001", } ] @@ -225,7 +225,7 @@ "stockrecords": [ { "price_currency": "EUR", - "price_excl_tax": "0.00", + "price": "0.00", "partner_sku": "sku002", } ] @@ -242,7 +242,24 @@ "stockrecords": [ { "price_currency": "EUR", - "price_excl_tax": "25.00", + "price": "25.00", + "partner_sku": "sku003", + } + ] + }, + { + "structure": "child", + "expires": "2017-01-01T12:00:00Z", + "attribute_values": [ + { + "name": "certificate_type", + "value": "verified" + } + ], + "stockrecords": [ + { + "price_currency": "EUR", + "price": "25.00", "partner_sku": "sku003", } ] @@ -260,7 +277,7 @@ "stockrecords": [ { "price_currency": "EUR", - "price_excl_tax": "250.00", + "price": "250.00", "partner_sku": "mobile.android.sku003", } ] @@ -277,7 +294,7 @@ "stockrecords": [ { "price_currency": "EUR", - "price_excl_tax": "25.00", + "price": "25.00", "partner_sku": "sku004" } ] @@ -304,7 +321,7 @@ "stockrecords": [ { "price_currency": "USD", - "price_excl_tax": "0.00", + "price": "0.00", "partner_sku": "sku005", } ] @@ -321,7 +338,7 @@ "stockrecords": [ { "price_currency": "USD", - "price_excl_tax": "25.00", + "price": "25.00", "partner_sku": "sku006", } ] @@ -350,7 +367,7 @@ "stockrecords": [ { "price_currency": "USD", - "price_excl_tax": "250.00", + "price": "250.00", "partner_sku": "sku007", } ] @@ -379,7 +396,7 @@ "stockrecords": [ { "price_currency": "USD", - "price_excl_tax": "250.00", + "price": "250.00", "partner_sku": "sku008", } ] @@ -404,7 +421,7 @@ "stockrecords": [ { "price_currency": "123", - "price_excl_tax": "0.00", + "price": "0.00", "partner_sku": "sku009", } ] @@ -429,7 +446,7 @@ "stockrecords": [ { "price_currency": "USD", - "price_excl_tax": "0.00", + "price": "0.00", "partner_sku": "sku010", } ] diff --git a/course_discovery/apps/course_metadata/data_loaders/tests/test_api.py b/course_discovery/apps/course_metadata/data_loaders/tests/test_api.py index 822848f648..9e8e2743ca 100644 --- a/course_discovery/apps/course_metadata/data_loaders/tests/test_api.py +++ b/course_discovery/apps/course_metadata/data_loaders/tests/test_api.py @@ -712,7 +712,7 @@ def mock_products_api(self, alt_course=None, alt_currency=None, alt_mode=None, h stockrecord = { "price_currency": alt_currency if alt_currency else "USD", - "price_excl_tax": "10.00", + "price": "10.00", } if valid_stockrecord: stockrecord.update({"partner_sku": "sku132"}) @@ -799,7 +799,7 @@ def assert_seats_loaded(self, body, mock_products): for product in products: stock_record = product['stockrecords'][0] price_currency = stock_record['price_currency'] - price = Decimal(stock_record['price_excl_tax']) + price = Decimal(stock_record['price']) sku = stock_record['partner_sku'] certificate_type = Seat.AUDIT credit_provider = None @@ -851,7 +851,7 @@ def assert_entitlements_loaded(self, body): course = Course.objects.get(uuid=attributes['UUID']) stock_record = datum['stockrecords'][0] price_currency = stock_record['price_currency'] - price = Decimal(stock_record['price_excl_tax']) + price = Decimal(stock_record['price']) sku = stock_record['partner_sku'] mode_name = attributes['certificate_type'] From 550e6427ec123c88763608b38a57baa10a2db37e Mon Sep 17 00:00:00 2001 From: Arunmozhi Date: Thu, 26 Sep 2024 10:09:47 +1000 Subject: [PATCH 5/5] fix: remove extra item added accidentally --- .../data_loaders/tests/mock_data.py | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/course_discovery/apps/course_metadata/data_loaders/tests/mock_data.py b/course_discovery/apps/course_metadata/data_loaders/tests/mock_data.py index 960440e2b2..fbc290c72e 100644 --- a/course_discovery/apps/course_metadata/data_loaders/tests/mock_data.py +++ b/course_discovery/apps/course_metadata/data_loaders/tests/mock_data.py @@ -247,23 +247,6 @@ } ] }, - { - "structure": "child", - "expires": "2017-01-01T12:00:00Z", - "attribute_values": [ - { - "name": "certificate_type", - "value": "verified" - } - ], - "stockrecords": [ - { - "price_currency": "EUR", - "price": "25.00", - "partner_sku": "sku003", - } - ] - }, # Mobile seat which we will discarded since we arn't syncing mobile skus see decicion 0025 { "structure": "child",