MCKIN-12934 Upgraded to django 2.x #217
Conversation
|
Thanks for the pull request, @nasirhjafri! I've created OSPR-4097 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams. Feel free to add as much of the following information to the ticket:
All technical communication about the code itself will still be done via the GitHub pull request interface. As a reminder, our process documentation is here. We can't start reviewing your pull request until you've submitted a signed contributor agreement or indicated your institutional affiliation. Please see the CONTRIBUTING file for more information. |
|
|
||
| # Notification type | ||
| msg_type = models.ForeignKey(SQLNotificationType, db_index=True) | ||
| msg_type = models.ForeignKey(SQLNotificationType, db_index=True, on_delete=models.DO_NOTHING) |
There was a problem hiding this comment.
I think we should use on_delete=models.CASCADE as it was the default value in older versions of Django. Here is the reference: https://docs.djangoproject.com/en/1.11/ref/models/fields/#django.db.models.ForeignKey.on_delete
We need to change this for each instance in this PR.
package.json
Outdated
| "karma-jasmine-jquery": "latest", | ||
| "karma-phantomjs-launcher": "latest", | ||
| "sinon": "latest", | ||
| "sinon": "<=7.5.0", |
There was a problem hiding this comment.
Is there any reason to pin sinon to a specific version? just need some context if there was any conflict or error?
There was a problem hiding this comment.
sinon 8 was released few weeks back and have some major changes which are causing the tests to break.
There was a problem hiding this comment.
@nasirhjafri that make sense. A little improvement could be to pin it like "sinon": "<8.0.0". We have already used this for rebase-ironwood branch and it is working for us.
requirements/base.txt
Outdated
| django==1.11.25 | ||
| git+https://github.com/edx/django-rest-framework.git@1ceda7c086fddffd1c440cc86856441bbf0bd9cb#egg=djangorestframework==3.6.3 | ||
| django>=2.0,<3.0 | ||
| djangorestframework==3.11.0 |
There was a problem hiding this comment.
We don't modify .txt requirement files directly now. We pin the versions is .in file and then use pip-compile to generate .txt file. you can pin your versions in .in file and generate .txt using command make upgrade.
For reference you can view the Makefile in project root.
setup.py
Outdated
| setup( | ||
| name='edx-notifications', | ||
| version='1.0.2', | ||
| version='1.0.3', |
There was a problem hiding this comment.
As this is a major release change, can you bump it to 1.1.0?
| url(r'^api/', include('edx_notifications.server.api.urls')), | ||
| url(r'^jsi18n/$', JavaScriptCatalog.as_view(), name='javascript-catalog'), | ||
| path('api/', include('edx_notifications.server.api.urls')), | ||
| path('jsi18n/', JavaScriptCatalog.as_view(), name='javascript-catalog'), |
There was a problem hiding this comment.
I need a little context about removing regex here. we could have used re_path with existing regex.
There was a problem hiding this comment.
path() always matches the complete path, so path('jsi18n/') is equivalent to url('^jsi18n/$')
So no need to use regexes over here.
nasirhjafri
force-pushed
the
MCKIN-12934
branch
2 times, most recently
from
a4a9542 to
1c7e28b
Compare
requirements/base.in
Outdated
| @@ -1,10 +1,10 @@ | |||
| # Django/Framework Packages | |||
| django==1.11.25 | |||
| django>=2.0 | |||
There was a problem hiding this comment.
@nasirhjafri please use django<2.3 instead
package.json
Outdated
| "karma-jasmine-jquery": "latest", | ||
| "karma-phantomjs-launcher": "latest", | ||
| "sinon": "latest", | ||
| "sinon": "<=7.5.0", |
There was a problem hiding this comment.
@nasirhjafri that make sense. A little improvement could be to pin it like "sinon": "<8.0.0". We have already used this for rebase-ironwood branch and it is working for us.
|
@ihtram Please review it again, thanks! |
|
@nasirhjafri Looks good now. Can you please squash the commits and push a single commit? |
nasirhjafri
force-pushed
the
MCKIN-12934
branch
from
c582bc8 to
2b99933
Compare
|
@ihtram Squashed the commit to a single commit. |
No description provided.