Django Oscar Upgrade to version 3.1 #4050

zubair-ce07 · 2023-11-12T06:40:12Z

⛔️ DEPRECATION WARNING

This repository is deprecated and in maintainence-only operation while we work on a replacement, please see this announcement for more information.

Although we have stopped integrating new contributions, we always appreciate security disclosures and patches sent to [email protected]

Anyone internally merging to this repository is expected to release and monitor their changes; if you are not able to do this DO NOT MERGE, please coordinate with someone who can to ensure that the changes are released.

Required Testing

Before deploying this change, complete a purchase in the stage environment.
(^ We can remove that manual check once REV-2624 is done and the corresponding e2e test runs again)

Description

Describe what this pull request changes, and why these changes were made. How will these changes affect other people, installations of edx, etc.?
Please include links to any relevant ADRs, design artifacts, and decision documents. Make sure to document the rationale behind significant changes in the repo, per OEP-19, and can be
linked here.

Useful information to include:

Which edX user roles will this change impact? Common user roles are "Learner", "Course Author", "Developer", and "Operator".
Include screenshots for changes to the UI (ideally, both "before" and "after" screenshots, if applicable).
Provide links to the description of corresponding configuration changes. Remember to correctly annotate these changes.

Supporting information

Link to other information about the change, such as Jira issues, GitHub issues, or Discourse discussions.
Be sure to check they are publicly readable, or if not, repeat the information here.

Testing instructions

Please provide detailed step-by-step instructions for testing this change; how did YOU test this change?

Other information

Include anything else that will help reviewers and consumers understand the change.

Does this change depend on other changes elsewhere?
Any special concerns or limitations? For example: deprecations, migrations, OpenEdx vs. edx.org differences, development vs. production environment differences, security, or accessibility.

awais786 · 2023-11-13T10:10:27Z

ecommerce/extensions/order/migrations/0026_auto_20231108_1355.py

@@ -0,0 +1,45 @@
+# Generated by Django 3.2.20 on 2023-11-08 13:55


This migration need changes to avoid price loss.

This migration is dropping 3 already deprecated fields from the Line model. From the read replica, I have found that we have data in these two columns (unit_retail_price and unit_cost_price). Out of 32 million records, only 384 rows have non-null/non-zero values in these columns.

I haven't found any reference in our codebase about these fields, so are we ok with the deletion of these fields or should we preserve them? Need suggestion from someone with better business domain knowledge.

cc @christopappas

For the sake of this PR, we (2U) wouldn't be affected, as this is just going to master. We when we make PRs for 2u/main branch, I think we should revisit this question then, particularly with the eyes of @dmsuter

That said, if there is an easy way to avoid this data from being lost for the sake of master branch, then I think for this PR we should do that out of courtesy for the community

Yes we can preserve these fields, we can add these fields to the forked version of this model here.

chore: updated factory dependency refactor: updated field name feat: Mgmt Command to create mobile seats for new course runs (#4046) fix: skipped a failing test. Will fix it in another ticket fix: updated method refactor: made changes as per new version of oscar refactor: updated code to make voucher name unique fix: removed white spaces fix: removed white spaces refactor: changed code as per new version of oscar refactor: updated code fix: override Product model in catalogue app fix: removed extra spaces fix: updated code fix: changes in code to pass checks fix: changes in code to pass checks

awais786 · 2023-11-14T12:00:51Z

Simple Checklist for sandbox testing:

Make sandbox with this branch and verify checkout working.
Make sandbox with master and then checkout this branch and make sure all migrations works as expected and checkout works.
No data loss due to migrations.
Add courses and coupons on sandbox.
verify ios and andriod apps owner informed about these changes. Few columns changed so it will effect those appps.

These changes are not reversible. So need proper testing from QA side.

iamsobanjaved · 2023-11-14T13:52:31Z

ecommerce/extensions/api/serializers.py

    """

    class Meta:
        model = StockRecord
-        fields = ('price_currency', 'price_excl_tax',)
+        fields = ('price_currency', 'price',)


It will break mobile apps

Changes discussed with mobile team

This will also break financial reporting; where is it being tested and when is this scheduled to be moved to production?

…mmerce into zubair-django-oscar31-update

awais786 · 2023-11-15T09:03:11Z

@zubair-ce07 @mumarkhan999 I think we can avoid vouchers change.
https://github.com/django-oscar/django-oscar/blob/97f95637ab8b4b1056f393774a3aee1fb7d7338e/src/oscar/apps/voucher/abstract_models.py#L102

We are overriding this model, so if declare field here without unique=True. We can avoid all vouchers updations. but need solid testing.

awais786 · 2023-11-16T08:29:27Z

ecommerce/extensions/api/v2/views/coupons.py

@@ -374,6 +374,13 @@ def update(self, request, *args, **kwargs):
    def update_voucher_data(self, request_data, vouchers):
        data = self.create_update_data_dict(data=request_data, fields=CouponVouchers.UPDATEABLE_VOUCHER_FIELDS)
        if data:
+            if 'name' in data:


You are exposing voucher code with name. Also any test case for this change ?

Django Oscar is also doing the same with old vouchers (appending voucher id to voucher name to make it unique). So I'm guessing it's safe to append voucher id. Regarding the test case, there is already one. We can update it according to the change.

awais786 · 2023-11-16T08:42:38Z

ecommerce/extensions/partner/migrations/0019_auto_20231108_1355.py

+            name='price_excl_tax',
+            field=models.DecimalField(blank=True, decimal_places=2, max_digits=12, null=True, verbose_name='Price'),
+        ),
+        migrations.RenameField(


This migration is moving data from old to new column

awais786 · 2023-11-16T08:46:09Z

ecommerce/extensions/voucher/migrations/0013_make_voucher_names_unique.py

+from django.db import migrations
+
+
+def make_voucher_names_unique(apps, schema_editor):


@mumarkhan999 u also need to add test for this migration.

mumarkhan999 · 2023-11-16T12:47:08Z

ecommerce/extensions/partner/migrations/0019_auto_20231108_1355.py

@@ -0,0 +1,49 @@
+# Generated by Django 3.2.20 on 2023-11-08 13:55


rawsql against this migration file

-- -- Remove field cost_price from historicalstockrecord -- ALTER TABLE `partner_historicalstockrecord` DROP COLUMN `cost_price`; -- -- Remove field price_retail from historicalstockrecord -- ALTER TABLE `partner_historicalstockrecord` DROP COLUMN `price_retail`; -- -- Remove field cost_price from stockrecord -- ALTER TABLE `partner_stockrecord` DROP COLUMN `cost_price`; -- -- Remove field price_retail from stockrecord -- ALTER TABLE `partner_stockrecord` DROP COLUMN `price_retail`; -- -- Alter field price_excl_tax on historicalstockrecord -- -- -- Rename field price_excl_tax on historicalstockrecord to price -- ALTER TABLE `partner_historicalstockrecord` RENAME COLUMN `price_excl_tax` TO `price`; -- -- Alter field price_excl_tax on stockrecord -- -- -- Rename field price_excl_tax on stockrecord to price -- ALTER TABLE `partner_stockrecord` RENAME COLUMN `price_excl_tax` TO `price`;

[inform] These tables have ~500k and ~77k records.

mumarkhan999 · 2023-11-16T12:49:15Z

ecommerce/extensions/voucher/migrations/0013_make_voucher_names_unique.py

+    Appends a number to voucher names.
+    """
+    Voucher = apps.get_model('voucher', 'Voucher')
+    vouchers = Voucher.objects.order_by('date_created')


Total number of PROD records affected by this data migration

…mmerce into zubair-django-oscar31-update

awais786 · 2023-12-11T11:01:14Z

This PR has ran these migrations on sandbox

  Applying analytics.0003_auto_20231108_1355... OK
  Applying catalogue.0056_auto_20231108_1355... OK
  Applying communication.0002_auto_20231108_1355... OK
  Applying customer.0008_auto_20231108_1355... OK
  Applying offer.0055_auto_20231108_1355... OK
  Applying order.0026_auto_20231108_1355... OK
  Applying partner.0019_auto_20231108_1355... OK
  Applying payment.0033_auto_20231108_1355... OK
  Applying voucher.0013_make_voucher_names_unique... OK
  Applying voucher.0014_auto_20231114_1156... OK

awais786 · 2023-12-15T14:48:41Z

ecommerce/extensions/catalogue/migrations/0027_catalogue_entitlement_option.py

@@ -4,21 +4,20 @@
 from django.db import migrations, models
 from oscar.core.loading import get_model

-Option = get_model('catalogue', 'Option')


@iamsobanjaved Please double check this migration.

This is fine, just getting the historical state of the model instead of the latest state.

awais786 · 2023-12-18T09:39:50Z

ecommerce/extensions/dashboard/offers/views.py

@@ -23,12 +23,10 @@ def _store_form_kwargs(self, form):
        session_data[self._key()] = json_data
        self.request.session.save()

-    def _fetch_form_kwargs(self, step_name=None):
+    def _fetch_form_kwargs(self):


django-oscar/django-oscar@1a3372e#diff-cfb952a27ceda33c67646e7222403395be5740581c6ff03bf9cbe8c28f83c63bL143

awais786 · 2023-12-18T10:32:34Z

ecommerce/templates/oscar/dashboard/orders/order_detail.html

@@ -1,738 +1,737 @@
 {% extends 'oscar/dashboard/layout.html' %}
 {% load i18n %}
-{% load compress %}


Why removed these ?

iamsobanjaved

I have reviewed all the changes and except one or two (mentioned in comments) everything looks fine. I couldn't review changes in the HTML files as I lacked context and domain knowledge.

iamsobanjaved · 2024-01-02T14:19:55Z

ecommerce/extensions/catalogue/models.py

@@ -62,6 +62,7 @@ def post_delete(self, instance, using=None, **kwargs):


 class Product(AbstractProduct):
+


Is this change required?

iamsobanjaved · 2024-01-05T13:01:55Z

ecommerce/extensions/dashboard/refunds/tests/test_acceptance.py

@@ -168,6 +168,7 @@ def test_processing_failure(self, approve):
            'Please try again, or contact the E-Commerce Development Team.'.format(refund_id=refund_id)
        )

+    @skip("Failing for some unknown reason, will fix it in another ticket.")


If we want to skip this one, we need to do manual QA for what this test is doing

iamsobanjaved · 2024-01-05T14:27:51Z

ecommerce/extensions/catalogue/migrations/0056_auto_20231108_1355.py

+        migrations.AlterField(
+            model_name='productattributevalue',
+            name='value_boolean',
+            field=models.BooleanField(blank=True, db_index=True, null=True, verbose_name='Boolean'),


[Inform] This table has 242k records in the database.

iamsobanjaved · 2024-01-05T14:32:41Z

ecommerce/extensions/customer/migrations/0008_auto_20231108_1355.py

+        migrations.AlterField(
+            model_name='productalert',
+            name='date_created',
+            field=models.DateTimeField(auto_now_add=True, db_index=True, verbose_name='Date created'),


[inform] This is adding db_index but we don't have any data in this model.

iamsobanjaved · 2024-01-05T15:05:40Z

ecommerce/extensions/order/migrations/0026_auto_20231108_1355.py

@@ -0,0 +1,45 @@
+# Generated by Django 3.2.20 on 2023-11-08 13:55


This migration is dropping 3 already deprecated fields from the Line model. From the read replica, I have found that we have data in these two columns (unit_retail_price and unit_cost_price). Out of 32 million records, only 384 rows have non-null/non-zero values in these columns.

I haven't found any reference in our codebase about these fields, so are we ok with the deletion of these fields or should we preserve them? Need suggestion from someone with better business domain knowledge.

cc @christopappas

iamsobanjaved · 2024-01-05T15:09:05Z

ecommerce/extensions/partner/migrations/0019_auto_20231108_1355.py

@@ -0,0 +1,49 @@
+# Generated by Django 3.2.20 on 2023-11-08 13:55


[inform] These tables have ~500k and ~77k records.

christopappas

Tests in the sandbox environment was successful, so I am giving my 👍 with 1 question:

Earlier in the review process there was mention of 1 column being deleted and recreated as a new name (instead of the column being simply renamed, therefore potentially leading to data loss).

Can someone confirm that is no longer an issue? If it is not an issue, we should be okay to merge.

zubair-ce07 · 2024-01-15T08:39:50Z

Tests in the sandbox environment was successful, so I am giving my 👍 with 1 question:

Earlier in the review process there was mention of 1 column being deleted and recreated as a new name (instead of the column being simply renamed, therefore potentially leading to data loss).

Can someone confirm that is no longer an issue? If it is not an issue, we should be okay to merge.

Yes we have changed that migration. Here is the migration path ecommerce/extensions/partner/migrations/0019_auto_20231108_1355.py

zubair-ce07 · 2024-01-16T09:33:59Z

All tests passed on sandbox. Also verified on local and everything is working fine. Merging this PR here.

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 handled the exception and falls back to the 'price' value. Ref: openedx-unsupported/ecommerce#4050

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: openedx-unsupported/ecommerce#4050

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 handles the exception and falls back to the 'price' value. Ref: openedx-unsupported/ecommerce#4050

* chore: bump upload-artifact GH task to v4 * 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: openedx-unsupported/ecommerce#4050 * refactor: apply review suggestion on fix * fix: update all references of `price_excl_tax` to price * fix: remove extra item added accidentally

chore: django oscar version upgrade to 3.1

4626bd8

zubair-ce07 marked this pull request as ready for review November 13, 2023 07:37

zubair-ce07 requested a review from a team as a code owner November 13, 2023 07:37

awais786 reviewed Nov 13, 2023

View reviewed changes

zubair-ce07 force-pushed the zubair-django-oscar31-update branch from 955e4d3 to a8acca8 Compare November 14, 2023 11:25

Merge branch 'master' into zubair-django-oscar31-update

6f0b745

mumarkhan999 force-pushed the zubair-django-oscar31-update branch 2 times, most recently from 99de787 to ddbfe3e Compare November 14, 2023 13:23

feat: add data migration to make voucher names unique

28f8cb6

mumarkhan999 force-pushed the zubair-django-oscar31-update branch from ddbfe3e to 28f8cb6 Compare November 14, 2023 13:33

iamsobanjaved reviewed Nov 14, 2023

View reviewed changes

zubair-ce07 added 3 commits November 15, 2023 11:47

fix: removed code

bd96751

Merge branch 'zubair-django-oscar31-update' of github.com:openedx/eco…

d56b082

…mmerce into zubair-django-oscar31-update

Merge branch 'master' into zubair-django-oscar31-update

4ef8517

awais786 reviewed Nov 16, 2023

View reviewed changes

mumarkhan999 reviewed Nov 16, 2023

View reviewed changes

zubair-ce07 added 6 commits November 16, 2023 18:34

refactor: updated django oscar templates

cad553c

Merge branch 'zubair-django-oscar31-update' of github.com:openedx/eco…

ef3a5c4

…mmerce into zubair-django-oscar31-update

Merge branch 'master' into zubair-django-oscar31-update

c68378a

Merge branch 'master' into zubair-django-oscar31-update

8fbe9ab

refactor: updated price field name

3e6b2e6

refactor: update price field name

e3860eb

awais786 reviewed Dec 15, 2023

View reviewed changes

awais786 reviewed Dec 18, 2023

View reviewed changes

iamsobanjaved approved these changes Jan 5, 2024

View reviewed changes

zubair-ce07 added 2 commits January 8, 2024 13:50

Merge branch 'master' into zubair-django-oscar31-update

b2aa8b7

feat: added refund functionality

d3ea7c0

christopappas approved these changes Jan 12, 2024

View reviewed changes

zubair-ce07 merged commit b8c214b into master Jan 16, 2024
8 of 10 checks passed

zubair-ce07 deleted the zubair-django-oscar31-update branch January 16, 2024 09:34

pshiu mentioned this pull request Mar 15, 2024

feat!: s/price_excl_tax/price for django oscar 3.1 upgrade openedx/course-discovery#4291

Closed

tecoholic mentioned this pull request Sep 23, 2024

fix: fallback to 'price' in ecommerce api loader open-craft/course-discovery#5

Merged

tecoholic mentioned this pull request Sep 24, 2024

fix: use "price" in ecommerce data loader openedx/course-discovery#4450

Merged

tecoholic mentioned this pull request Oct 4, 2024

Copy of - fix: use "price" in ecommerce data loader (#4450) openedx/course-discovery#4459

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Django Oscar Upgrade to version 3.1 #4050

Django Oscar Upgrade to version 3.1 #4050

zubair-ce07 commented Nov 12, 2023

awais786 Nov 13, 2023 •

edited

Loading

iamsobanjaved Jan 5, 2024

christopappas Jan 8, 2024

iamsobanjaved Jan 12, 2024

awais786 commented Nov 14, 2023 •

edited

Loading

iamsobanjaved Nov 14, 2023

zubair-ce07 Nov 16, 2023

dmsuter Nov 16, 2023

awais786 commented Nov 15, 2023

awais786 Nov 16, 2023 •

edited

Loading

zubair-ce07 Nov 16, 2023

awais786 Nov 16, 2023

awais786 Nov 16, 2023

mumarkhan999 Nov 16, 2023

iamsobanjaved Jan 5, 2024

mumarkhan999 Nov 16, 2023

awais786 commented Dec 11, 2023

awais786 Dec 15, 2023

iamsobanjaved Dec 18, 2023

awais786 Dec 18, 2023

awais786 Dec 18, 2023

iamsobanjaved left a comment

iamsobanjaved Jan 2, 2024

iamsobanjaved Jan 5, 2024

iamsobanjaved Jan 5, 2024

iamsobanjaved Jan 5, 2024

iamsobanjaved Jan 5, 2024

iamsobanjaved Jan 5, 2024

christopappas left a comment

zubair-ce07 commented Jan 15, 2024

zubair-ce07 commented Jan 16, 2024

		@@ -0,0 +1,45 @@
		# Generated by Django 3.2.20 on 2023-11-08 13:55

		from django.db import migrations


		def make_voucher_names_unique(apps, schema_editor):

		@@ -0,0 +1,49 @@
		# Generated by Django 3.2.20 on 2023-11-08 13:55

		@@ -62,6 +62,7 @@ def post_delete(self, instance, using=None, **kwargs):


		class Product(AbstractProduct):

Django Oscar Upgrade to version 3.1 #4050

Django Oscar Upgrade to version 3.1 #4050

Conversation

zubair-ce07 commented Nov 12, 2023

⛔️ DEPRECATION WARNING

Anyone internally merging to this repository is expected to release and monitor their changes; if you are not able to do this DO NOT MERGE, please coordinate with someone who can to ensure that the changes are released.

Required Testing

Description

Supporting information

Testing instructions

Other information

awais786 Nov 13, 2023 • 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

awais786 commented Nov 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awais786 commented Nov 15, 2023

awais786 Nov 16, 2023 • 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

awais786 commented Dec 11, 2023

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

iamsobanjaved 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christopappas left a comment

Choose a reason for hiding this comment

zubair-ce07 commented Jan 15, 2024

zubair-ce07 commented Jan 16, 2024

awais786 Nov 13, 2023 •

edited

Loading

awais786 commented Nov 14, 2023 •

edited

Loading

awais786 Nov 16, 2023 •

edited

Loading