Feat: Upgrade django-oscar to version 3.2 #4023

zubair-ce07 · 2023-08-31T12:51:39Z

Description

Changes related to upgrade ecommerce application to django-oscar version 3.2 from 2.2

Required Testing

Thoroughly test ecommerce dashboard. (Every flow needs to be tested)

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.

Other information

Ticket: https://2u-internal.atlassian.net/browse/REV-3639

julianajlk

Thanks for doing this work! Question - are the template changes included in this PR part of the upgrade? Were you able to see the changes locally/do they make sense?

ecommerce/templates/oscar/dashboard/catalogue/product_update.html

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

christopappas · 2023-09-07T16:54:57Z

i see that the tests are failing with somenthing related to option.OPTIONAL

not sure if you've been digging into this yet, but, in my local ecommerce i see OPTIONAL as an attribute of the AbstactOption class

root@ecommerce:/edx/app/ecommerce/ecommerce# grep 'AbstractOption' -A 35  /edx/app/ecommerce/venvs/ecommerce/lib/python3.8/site-packages/oscar/apps/catalogue/abstract_models.py 
class AbstractOption(models.Model):
    """
    An option that can be selected for a particular item when the product
    is added to the basket.

    For example,  a list ID for an SMS message send, or a personalised message
    to print on a T-shirt.

    This is not the same as an 'attribute' as options do not have a fixed value
    for a particular item.  Instead, option need to be specified by a customer
    when they add the item to their basket.
    """
    name = models.CharField(_("Name"), max_length=128)
    code = AutoSlugField(_("Code"), max_length=128, unique=True,
                         populate_from='name')

    REQUIRED, OPTIONAL = ('Required', 'Optional')

looks like optional was deleted in this commit: django-oscar/django-oscar@dd5a151#diff-6101f645bcf6dd2482680b01d02acf8a6010004e7a7730a9513e9cdc41165aff

i think for backwards compatibility, we may want to add OPTIONAL with text of 'Optional' to https://github.com/openedx/ecommerce/blob/master/ecommerce/extensions/catalogue/models.py#L179

zubair-ce07 · 2023-09-08T13:36:55Z

Thanks for doing this work! Question - are the template changes included in this PR part of the upgrade? Were you able to see the changes locally/do they make sense?

Yes Otherwise whole pages alignment were out of order. We need to upgrade template pages as per new upgraded version.

christopappas · 2023-10-18T18:51:26Z

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

@@ -32,5 +32,35 @@ class Migration(migrations.Migration):
    ]

    operations = [
+        migrations.AddField(


was this the old migration we're adding to? why do we need to do this? this seems odd to me... i'm worried that changing a migration that has already been applied 1) would not have any effect 2) break the application of migrations

christopappas · 2023-10-18T19:04:02Z

ecommerce/extensions/voucher/utils.py

@@ -537,8 +537,9 @@ def create_new_voucher(code, end_datetime, name, start_datetime, voucher_type):
    if not isinstance(end_datetime, datetime.datetime):
        end_datetime = dateutil.parser.parse(end_datetime)

+    name = name[:128 - len(voucher_code)] + voucher_code


christopappas

Thanks for all this work Zubair. Before I 👍 , I'd like to understand the update to the old migration file and why that is necessary

christopappas · 2023-10-20T20:47:17Z

ecommerce/extensions/basket/migrations/0016_alter_lineattribute_value.py

@@ -0,0 +1,19 @@
+# Generated by Django 3.2.20 on 2023-08-24 10:28


so it's definitely weird that this migration and other other one are not gettign covered by codecov. these were also made a few months back. i wonder if there's somtehing about changes we have made along the way that has prevented these migrations from being relevant? i honestly am not sure. if we delete this migrations from you branch, re run makemigrations, and these appear again , and then you push them up this branch and we still see this coverage issue, then i think it might be fine. i just want to make sure there isn't anything extra that was created along the way that is no longer relevant 🤷 -- honestly though, i'm not certain why this is the case

this is the case for so many of the migrations 🤔 -- that's so odd...

christopappas · 2023-10-20T20:48:23Z

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')


was this change still necessary based on the latest approach you described?

Hy @christopappas. Yes this is necessary otherwise it will load the latest Option model (One created after running all the migrations) giving error "required field is not defined" because at this point required column is not created in our db.

christopappas · 2023-10-20T20:52:27Z

you may need to squash your commits btw. looks like there's a commitlint error

christopappas

i'm giving you a 👍 because i think you have done enough due diligence and QA here so far. if none of the most recent comments I make you feel like something to change, and if you are comfortable with the stage of this, i would like to see us merge this and test extensively on stage 🚀

This reverts commit 4d275df.

* feat: changes to upgrad django-oscar to version 3.2 * chore: upgraded django oscar to version 3.2

This reverts commit 4d275df.

feat: changes to upgrad django-oscar to version 3.2

c1436fa

zubair-ce07 requested a review from a team as a code owner August 31, 2023 12:51

julianajlk reviewed Sep 7, 2023

View reviewed changes

ecommerce/templates/oscar/dashboard/catalogue/product_update.html Outdated Show resolved Hide resolved

ecommerce/templates/oscar/dashboard/orders/order_list.html Show resolved Hide resolved

zubair-ce07 force-pushed the zubair-django-oscar-upgrade branch from 8040119 to 3fee834 Compare September 21, 2023 10:50

christopappas reviewed Oct 18, 2023

View reviewed changes

christopappas reviewed Oct 20, 2023

View reviewed changes

christopappas approved these changes Oct 20, 2023

View reviewed changes

chore: upgraded django oscar to version 3.2

7d6a998

zubair-ce07 force-pushed the zubair-django-oscar-upgrade branch from a417de8 to 7d6a998 Compare October 24, 2023 07:18

Merge branch 'master' into zubair-django-oscar-upgrade

39fe6ed

zubair-ce07 merged commit 4d275df into master Oct 24, 2023
6 of 8 checks passed

zubair-ce07 deleted the zubair-django-oscar-upgrade branch October 24, 2023 08:17

zubair-ce07 added a commit that referenced this pull request Oct 24, 2023

Revert "Feat: Upgrade django-oscar to version 3.2 (#4023)"

4952bdd

This reverts commit 4d275df.

zubair-ce07 mentioned this pull request Oct 24, 2023

Revert "Feat: Upgrade django-oscar to version 3.2" #4043

Merged

christopappas pushed a commit that referenced this pull request Oct 24, 2023

Revert "Feat: Upgrade django-oscar to version 3.2 (#4023)" (#4043)

0a7a61f

This reverts commit 4d275df.

christopappas pushed a commit that referenced this pull request Dec 4, 2023

Feat: Upgrade django-oscar to version 3.2 (#4023)

8057c62

* feat: changes to upgrad django-oscar to version 3.2 * chore: upgraded django oscar to version 3.2

christopappas pushed a commit that referenced this pull request Dec 4, 2023

Revert "Feat: Upgrade django-oscar to version 3.2 (#4023)" (#4043)

110bfa7

This reverts commit 4d275df.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: Upgrade django-oscar to version 3.2 #4023

Feat: Upgrade django-oscar to version 3.2 #4023

zubair-ce07 commented Aug 31, 2023

julianajlk left a comment

christopappas commented Sep 7, 2023

zubair-ce07 commented Sep 8, 2023

christopappas Oct 18, 2023

christopappas Oct 18, 2023

christopappas left a comment

christopappas Oct 20, 2023

christopappas Oct 20, 2023

christopappas Oct 20, 2023

zubair-ce07 Oct 23, 2023

christopappas commented Oct 20, 2023

christopappas left a comment

		@@ -0,0 +1,19 @@
		# Generated by Django 3.2.20 on 2023-08-24 10:28

Feat: Upgrade django-oscar to version 3.2 #4023

Feat: Upgrade django-oscar to version 3.2 #4023

Conversation

zubair-ce07 commented Aug 31, 2023

Description

Required Testing

Supporting information

Other information

julianajlk left a comment

Choose a reason for hiding this comment

christopappas commented Sep 7, 2023

zubair-ce07 commented Sep 8, 2023

christopappas Oct 18, 2023

Choose a reason for hiding this comment

christopappas Oct 18, 2023

Choose a reason for hiding this comment

christopappas left a comment

Choose a reason for hiding this comment

christopappas Oct 20, 2023

Choose a reason for hiding this comment

christopappas Oct 20, 2023

Choose a reason for hiding this comment

christopappas Oct 20, 2023

Choose a reason for hiding this comment

zubair-ce07 Oct 23, 2023

Choose a reason for hiding this comment

christopappas commented Oct 20, 2023

christopappas left a comment

Choose a reason for hiding this comment