asadiqbal08/ Django Version bump

[asadiqbal08]

Pre-Flight checklist

• Testing
  • Code is tested
  • Changes have been manually tested

What are the relevant tickets?

fixes: #2333

How should this be manually tested?

Build the container with latest version bump.
Test the web application behavior specially the Wagtail part.

[codecov-commenter]

Codecov Report

Merging #2343 (4f9792a) into master (19031f3) will increase coverage by 0.07%.
The diff coverage is 92.30%.

@@            Coverage Diff             @@
##           master    #2343      +/-   ##
==========================================
+ Coverage   88.21%   88.28%   +0.07%     
==========================================
  Files         330      330              
  Lines       15098    15098              
  Branches     1702     1703       +1     
==========================================
+ Hits        13318    13330      +12     
+ Misses       1532     1518      -14     
- Partials      248      250       +2     

Impacted Files	Coverage Δ
mitxpro/utils.py	99.39% <80.00%> (-0.61%)	⬇️
b2b_ecommerce/models.py	93.82% <100.00%> (-0.08%)	⬇️
cms/embeds.py	46.42% <100.00%> (ø)
cms/models.py	91.32% <100.00%> (ø)
ecommerce/models.py	94.98% <100.00%> (-0.02%)	⬇️
mitxpro/models.py	93.18% <100.00%> (ø)
mitxpro/settings.py	92.41% <100.00%> (+0.02%)	⬆️
...tic/js/components/forms/BulkEnrollmentForm_test.js	98.73% <0.00%> (-1.27%)	⬇️
localdev/seed/api.py	76.84% <0.00%> (-0.36%)	⬇️
users/apps.py	100.00% <0.00%> (+100.00%)	⬆️
... and 4 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 19031f3...4f9792a. Read the comment docs.

[arslanashraf7]

@asadiqbal08 The Python tests in the CI are failing due to lint errors, can you check?

[asadiqbal08]

@pdpinch After investigating the issue related to test failure, I tried to traced it that was producing the following exception:

File "/usr/local/lib/python3.7/site-packages/django/apps/config.py", line 189, in create
    warnings.warn(message, RemovedInDjango41Warning, stacklevel=2)
django.utils.deprecation.RemovedInDjango41Warning: 'social_django' defines default_app_config = 'social_django.config.PythonSocialAuthConfig'. However, Django's automatic detection did not find this configuration. You should move the default config class to the apps submodule of your application and, if this module defines several config classes, mark the default one with default = True.

So I figured out that we need to bump the social-auth-app-django to latest version that is 5.0.0 in which they actually added some checks for the default_app_config to be conditional on basis of Django version.

As per Django=3.2 release notes --> With automatic AppConfig discovery, default_app_config is no longer needed.

So the problem here is that, we could not pinned it like social-auth-app-django==5.0.0 as the latest version of Djoser is not supporting Django==3.2. I had a discussion with @umarmughal824 and came to know that @umarmughal824 had a PR for Django 3.2 upgrade in both djoser and social-auth applications.

• add django 3.2 support sunscrapers/djoser#633 that seems to me it is not released.
• fix auto field warning for django 3.2 python-social-auth/social-app-django#354

So that is actually, I guess, blocking me to this upgrade in mitxPro.

[asadiqbal08]

Ok continuing further to this, I have some findings and suggestions to continue over this part.

1- As we had pointed out over the PR for releasing the changes, (That may take time from their side ) meanwhile, we are thinking to filter out the above Django warning RemovedInDjango41Warning in mitxpro by following the same practice as done for other warning HERE. ✅

2- For migrating to Django=3.2, We need to upgrade the version of Wagtail to either 3.2 or later. Difference shown for Django support in v2.12.6 VS v2.13 compatibility report.

3- With this wagtail upgrade as mentioned in point#2 from v2.12.6 to v2.13, Wagtail introduced some new models as an example the Comment model. That requires to run the migration changes for wagtail.

4- The following migration 0051_new_page_data_migrations run before the wagtail migrations and try to migrate data which causes the following exception.

5- So moving ahead, Should we consider to move this migration logic from the migration file to under some management command ? I am not sure what were the actual reasons to keep this stuff inside the migration files.

@pdpinch @arslanashraf7 thoughts ?

[pdpinch]

Is this comment in the migration file relevant? It suggests changing the legacy migration to a no-op and adding a new one.

https://github.com/mitodl/mitxpro/blob/master/cms/migrations/0051_new_page_data_migrations.py#L10-L13

[asadiqbal08]

@pdpinch as a good practise, I think these stuff should not be part of migration files. Maintaining migration files manually is not be good.

[arslanashraf7]

Thanks for making the requested changes @asadiqbal08. The warnings seem to be resolved now and I was able to test the app now.
However, I added some minor comments & I'm seeing errors two types of errors with images:

1. All the images from the wagtail are not showing on the app pages, the static images work fine.
2. Open /cms and visit Images menu or any pages that contain Image field, you will be able to notice errors regarding django-redis.

I believe the errors are related to Image Renditions which use django-redis as a storage backend.

Error Logs

nginx_1 | 172.21.0.1 - - [23/Feb/2022:11:50:52 +0000] "GET /cms/pages/34/edit/ HTTP/1.1" 500 644930 "http://xpro.odl.local:8053/cms/pages/29/" "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/98.0.4758.102 Safari/537.36" "-" web_1 | File "/usr/local/lib/python3.7/site-packages/django/template/base.py", line 796, in resolve web_1 | value = self._resolve_lookup(context) web_1 | File "/usr/local/lib/python3.7/site-packages/django/template/base.py", line 858, in _resolve_lookup web_1 | current = current() web_1 | File "/usr/local/lib/python3.7/site-packages/wagtail/admin/edit_handlers.py", line 496, in render_as_object web_1 | 'show_add_comment_button': self.comments_enabled and getattr(self.bound_field.field.widget, 'show_add_comment_button', True), web_1 | File "/usr/local/lib/python3.7/site-packages/django/template/loader.py", line 62, in render_to_string web_1 | return template.render(context, request) web_1 | File "/usr/local/lib/python3.7/site-packages/django/template/backends/django.py", line 61, in render web_1 | return self.template.render(context) web_1 | File "/usr/local/lib/python3.7/site-packages/django/template/base.py", line 170, in render web_1 | return self._render(context) web_1 | File "/usr/local/lib/python3.7/site-packages/django/test/utils.py", line 100, in instrumented_test_render web_1 | return self.nodelist.render(context) web_1 | File "/usr/local/lib/python3.7/site-packages/django/template/base.py", line 938, in render web_1 | bit = node.render_annotated(context) web_1 | File "/usr/local/lib/python3.7/site-packages/django/template/base.py", line 905, in render_annotated web_1 | return self.render(context) web_1 | File "/usr/local/lib/python3.7/site-packages/django/template/loader_tags.py", line 195, in render web_1 | return template.render(context) web_1 | File "/usr/local/lib/python3.7/site-packages/django/template/base.py", line 172, in render web_1 | return self._render(context) web_1 | File "/usr/local/lib/python3.7/site-packages/django/test/utils.py", line 100, in instrumented_test_render web_1 | return self.nodelist.render(context) web_1 | File "/usr/local/lib/python3.7/site-packages/django/template/base.py", line 938, in render web_1 | bit = node.render_annotated(context) web_1 | File "/usr/local/lib/python3.7/site-packages/django/template/base.py", line 905, in render_annotated web_1 | return self.render(context) web_1 | File "/usr/local/lib/python3.7/site-packages/django/template/loader_tags.py", line 53, in render web_1 | result = self.nodelist.render(context) web_1 | File "/usr/local/lib/python3.7/site-packages/django/template/base.py", line 938, in render web_1 | bit = node.render_annotated(context) web_1 | File "/usr/local/lib/python3.7/site-packages/django/template/base.py", line 905, in render_annotated web_1 | return self.render(context) web_1 | File "/usr/local/lib/python3.7/site-packages/django/template/base.py", line 988, in render web_1 | output = self.filter_expression.resolve(context) web_1 | File "/usr/local/lib/python3.7/site-packages/django/template/base.py", line 698, in resolve web_1 | new_obj = func(obj, *arg_vals) web_1 | File "/usr/local/lib/python3.7/site-packages/wagtail/admin/templatetags/wagtailadmin_tags.py", line 261, in render_with_errors web_1 | return bound_field.as_widget() web_1 | File "/usr/local/lib/python3.7/site-packages/django/forms/boundfield.py", line 97, in as_widget web_1 | renderer=self.form.renderer, web_1 | File "/usr/local/lib/python3.7/site-packages/wagtail/core/blocks/base.py", line 518, in render web_1 | return self.render_with_errors(name, value, attrs=attrs, errors=None, renderer=renderer) web_1 | File "/usr/local/lib/python3.7/site-packages/wagtail/core/blocks/base.py", line 500, in render_with_errors web_1 | value_json = json.dumps(self.block_def.get_form_state(value)) web_1 | File "/usr/local/lib/python3.7/site-packages/wagtail/core/blocks/stream_block.py", line 264, in get_form_state web_1 | for child in value web_1 | File "/usr/local/lib/python3.7/site-packages/wagtail/core/blocks/stream_block.py", line 264, in <listcomp> web_1 | for child in value web_1 | File "/usr/local/lib/python3.7/site-packages/wagtail/core/blocks/struct_block.py", line 200, in get_form_state web_1 | for name, val in value.items() web_1 | File "/usr/local/lib/python3.7/site-packages/wagtail/core/blocks/struct_block.py", line 200, in <listcomp> web_1 | for name, val in value.items() web_1 | File "/usr/local/lib/python3.7/site-packages/wagtail/images/blocks.py", line 22, in get_form_state web_1 | value_data = self.widget.get_value_data(value) web_1 | File "/usr/local/lib/python3.7/site-packages/wagtail/images/widgets.py", line 34, in get_value_data web_1 | preview_image = get_rendition_or_not_found(image, 'max-165x165') web_1 | File "/usr/local/lib/python3.7/site-packages/wagtail/images/shortcuts.py", line 13, in get_rendition_or_not_found web_1 | return image.get_rendition(specs) web_1 | File "/usr/local/lib/python3.7/site-packages/wagtail/images/models.py", line 292, in get_rendition web_1 | cached_rendition = cache.get(rendition_cache_key) web_1 | File "/usr/local/lib/python3.7/site-packages/django_redis/cache.py", line 91, in get web_1 | value = self._get(key, default, version, client) web_1 | File "/usr/local/lib/python3.7/site-packages/django_redis/cache.py", line 31, in _decorator web_1 | return method(self, *args, **kwargs) web_1 | File "/usr/local/lib/python3.7/site-packages/django_redis/cache.py", line 98, in _get web_1 | return self.client.get(key, default=default, version=version, client=client) web_1 | File "/usr/local/lib/python3.7/site-packages/django_redis/client/default.py", line 265, in get web_1 | return self.decode(value) web_1 | File "/usr/local/lib/python3.7/site-packages/django_redis/client/default.py", line 432, in decode web_1 | value = self._serializer.loads(value) web_1 | File "/usr/local/lib/python3.7/site-packages/django_redis/serializers/pickle.py", line 27, in loads web_1 | return pickle.loads(value) web_1 | File "/usr/local/lib/python3.7/site-packages/django/db/models/fields/files.py", line 142, in __setstate__ web_1 | self.storage = self.field.storage web_1 | AttributeError: 'ImageFieldFile' object has no attribute 'field'

[arslanashraf7]

I think we should try to take the wagtail to the latest version if possible. If not, we should at least get to the latest for 2.13 which is I think 2.13.5 ATM.

[arslanashraf7]

@asadiqbal08 What's your opinion on this?

[asadiqbal08]

@arslanashraf7 I tested it again as you suggested in 1-1 call today but strange it is not happening for me.
Here are the steps that followed.

1- Down all images/containers.
2- Checkout to master and build on master
3- docker-compose up and add images in CMS.
4- checkout this Branch/PR and build again.
5- docker-compose up and visited CMS.
6- Uploaded images and added to contents.
I did not see any exception over the console and nothing break. please let me know If I missed any steps above ?

[asadiqbal08]

After looking around much, We figured it out that the above exception that shared by @arslanashraf7 above is getting out due to wagtail image renditions. MitxPro is utilizing the Redis memory cache to hold wagtail images that are prefixed with WAG* that was introduced by Gavin in PR Here

As we updated the redis version in this PR and In order to resolve this, I have to Flushall the redis cache to retest it and then it works fine and @arslanashraf7 has also confirmed that.

@blarghmatey keeping you in loop here to be sure if an exception raised over some other environment e.g. prod then we may need to delete the Waigtail image renditions ,with prefix WAG*, from Redis cache to fix it.

redis-cli KEYS "WAG:*" | xargs redis-cli DEL

Images that are cached over my local instance:

cc:
@pdpinch

[arslanashraf7]

@asadiqbal08 The things are looking in good shape now the errors that I mentioned above go away by running redis-cli FLUSHALL in the Redis container but when I run redis-cli KEYS "WAG:*" | xargs redis-cli DEL i see:

(error) ERR wrong number of arguments for 'keys' command

(Maybe this is because I don't have any keys although I have rendition objects).

Since I was still seeing the error without running FLUSHALL, I looked around wagtail a bit and i think there is one other possibility to fix this issue around. What i did was delete all the rendition objects created by wagtails (Of course these will regenerate once the images will be loaded again):

from wagtail.images.models import Rendition
Rendition.objects.all().delete()

What are your thoughts on this between using this solution and the above-mentioned redis-cli solution?

[asadiqbal08]

@arslanashraf7 I am thinking to remove renditions only with Prefix WAG:*, I am not sure the command that you shared will do the same job or not.
Do you have still any concerns or should we go ahead to merge it ?

cc: @pdpinch @blarghmatey

[arslanashraf7]

@arslanashraf7 I am thinking to remove renditions only with Prefix WAG:*, I am not sure the command that you shared will do the same job or not. Do you have still any concerns or should we go ahead to merge it ?

@asadiqbal08 I shared the model-based script for Django shell. That uses the Rendition model from wagtail itself and removed the cache/rendition entries. However, I'm still fine if you want to go with the command you mentioned above and it worked, my solution was an alternate approach.

[arslanashraf7]

Thanks for making the requested changes. 👍

[asadiqbal08]

@blarghmatey @pdpinch do you have any concerns before merging this PR ?

[blarghmatey]

Nothing from my side