This repository has been archived by the owner. It is now read-only.

Port to Django 1.6 #1769

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@washort
Contributor

washort commented Feb 15, 2014

These are the last few changes needed for Django 1.6 compatibility.
PRs for dependencies:
jbalogh/test-utils#25
mozilla/nuggets#4

requirements/prod.txt
dj-database-url==0.2.2
django-aesfield==0.1.2
django-browserid==0.8
-django-cache-machine==0.8

This comment has been minimized.

@diox

diox Feb 15, 2014

Member

For every requirement you had to fork, we need to push for a new release with your fixes. Let's try to avoid keeping forks around.

@diox

diox Feb 15, 2014

Member

For every requirement you had to fork, we need to push for a new release with your fixes. Let's try to avoid keeping forks around.

This comment has been minimized.

@washort

washort Feb 15, 2014

Contributor

links for dependency PRs added.

@washort

washort Feb 15, 2014

Contributor

links for dependency PRs added.

@diox

This comment has been minimized.

Show comment
Hide comment
@diox

diox Feb 15, 2014

Member

So, I realize this is going to be annoying, but I think we need to split this PR up. I'd like to have at least the version-related changes and the fixtures changes in separate PRs to make it easier to review and spot regressions.

The most backwards-compatible changes we can merge before the actual switch to 1.6, the better.

Member

diox commented Feb 15, 2014

So, I realize this is going to be annoying, but I think we need to split this PR up. I'd like to have at least the version-related changes and the fixtures changes in separate PRs to make it easier to review and spot regressions.

The most backwards-compatible changes we can merge before the actual switch to 1.6, the better.

@washort

This comment has been minimized.

Show comment
Hide comment
@washort

washort Feb 15, 2014

Contributor

I agree that splitting things up is better. I'll see what I can do.

Contributor

washort commented Feb 15, 2014

I agree that splitting things up is better. I'll see what I can do.

@washort

This comment has been minimized.

Show comment
Hide comment
@washort

washort Feb 18, 2014

Contributor

preliminary changes moved to mozilla/zamboni#1774

Contributor

washort commented Feb 18, 2014

preliminary changes moved to mozilla/zamboni#1774

@washort

This comment has been minimized.

Show comment
Hide comment
@washort

washort Feb 18, 2014

Contributor

preliminary changes merged.

Contributor

washort commented Feb 18, 2014

preliminary changes merged.

apps/addons/tests/test_models.py
@@ -2199,6 +2199,7 @@ def setUp(self):
def test_extract(self):
File.objects.create(platform=self.platform_mob, version=self.version,
filename=self.xpi_path('langpack-localepicker'))
+ self.addon.update(_current_version=self.version)

This comment has been minimized.

@diox

diox Feb 24, 2014

Member

This shouldn't be necessary with the removal of the version magic, or something is wrong.

@diox

diox Feb 24, 2014

Member

This shouldn't be necessary with the removal of the version magic, or something is wrong.

This comment has been minimized.

@washort

washort Feb 24, 2014

Contributor

Something must be wrong then, because removing this causes get_localepicker to fail.

@washort

washort Feb 24, 2014

Contributor

Something must be wrong then, because removing this causes get_localepicker to fail.

This comment has been minimized.

@diox

diox Feb 25, 2014

Member

Even if you self.addon.reload() ?

Edit:* It could be because the File we create has the wrong status actually.

@diox

diox Feb 25, 2014

Member

Even if you self.addon.reload() ?

Edit:* It could be because the File we create has the wrong status actually.

This comment has been minimized.

@washort

washort Feb 25, 2014

Contributor

reload() works here without that update. Oops, no it doesn't. I'll have a look at the file status.

@washort

washort Feb 25, 2014

Contributor

reload() works here without that update. Oops, no it doesn't. I'll have a look at the file status.

apps/addons/tests/test_models.py
@@ -2213,6 +2214,7 @@ def test_extract_no_files(self):
def test_extract_not_language_pack(self):
File.objects.create(platform=self.platform_mob, version=self.version,
filename=self.xpi_path('langpack-localepicker'))
+ self.addon.update(_current_version=self.version)

This comment has been minimized.

@diox

diox Feb 24, 2014

Member

See above.

@diox

diox Feb 24, 2014

Member

See above.

apps/api/tests/test_views.py
@@ -1283,6 +1283,7 @@ def setup_localepicker(self, platform):
self.addon.update(type=amo.ADDON_LPAPP, status=amo.STATUS_PUBLIC)
version = self.addon.versions.all()[0]
File.objects.create(version=version, platform_id=platform)
+ self.addon.update(_current_version=version)

This comment has been minimized.

@diox

diox Feb 24, 2014

Member

Again shouldn't be necessary.

@diox

diox Feb 24, 2014

Member

Again shouldn't be necessary.

apps/users/models.py
@@ -312,7 +312,9 @@ def last_login(self):
@amo.cached_property
def reviews(self):
"""All reviews that are not dev replies."""
- return self._reviews_all.filter(reply_to=None)
+ qs = self._reviews_all.filter(reply_to=None)
+ list(qs) # This line is magic. Delete it.

This comment has been minimized.

@diox

diox Feb 24, 2014

Member

wat ?

@diox

diox Feb 24, 2014

Member

wat ?

This comment has been minimized.

@washort

washort Feb 24, 2014

Contributor

I should try deleting that now, yeah :)

@washort

washort Feb 24, 2014

Contributor

I should try deleting that now, yeah :)

This comment has been minimized.

@washort

washort Feb 25, 2014

Contributor

absence of this line causes several reviews tests to hang. I'll add a more informative comment and we can tackle the source of it later.

@washort

washort Feb 25, 2014

Contributor

absence of this line causes several reviews tests to hang. I'll add a more informative comment and we can tackle the source of it later.

lib/misc/safe_signals.py
@@ -28,7 +28,7 @@ def safe_send(self, sender, **named):
# Call each receiver with whatever arguments it can accept.
# Return a list of tuple pairs [(receiver, response), ... ].
- for receiver in self._live_receivers(_make_id(sender)):
+ for receiver in self._live_receivers(sender):

This comment has been minimized.

@diox

diox Feb 24, 2014

Member

We should remove lib/misc/safe_signals entirely. It shouldn't be used anymore.

@diox

diox Feb 24, 2014

Member

We should remove lib/misc/safe_signals entirely. It shouldn't be used anymore.

@@ -70,10 +69,6 @@
if not settings.DEBUG:
warnings.simplefilter('ignore')
-# The first thing execute_manager does is call `setup_environ`. Logging config
-# needs to access settings, so we'll setup the environ early.
-setup_environ(settings)

This comment has been minimized.

@diox

diox Feb 24, 2014

Member

Slightly nervous about this since there is a few things going on below that might need settings...

@diox

diox Feb 24, 2014

Member

Slightly nervous about this since there is a few things going on below that might need settings...

This comment has been minimized.

@washort

washort Feb 24, 2014

Contributor

Just importing settings is enough now.

@washort

washort Feb 24, 2014

Contributor

Just importing settings is enough now.

mkt/developers/views.py
@@ -835,6 +834,7 @@ def addons_section(request, addon_id, addon, section, editable=False,
all_forms.append(additional_form)
if all(not f or f.is_valid() for f in all_forms):
+ addon.save()

This comment has been minimized.

@diox

diox Feb 24, 2014

Member

why ?

@diox

diox Feb 24, 2014

Member

why ?

This comment has been minimized.

@washort

washort Feb 25, 2014

Contributor

mkt.developers.tests.test_views_edit.TestEditTechnical.test_public_stats fails without this

@washort

washort Feb 25, 2014

Contributor

mkt.developers.tests.test_views_edit.TestEditTechnical.test_public_stats fails without this

This comment has been minimized.

@diox

diox Feb 25, 2014

Member

There is a form.save() below so it really shouldn't be necessary, we should figure out what's wrong with the test

@diox

diox Feb 25, 2014

Member

There is a form.save() below so it really shouldn't be necessary, we should figure out what's wrong with the test

This comment has been minimized.

@diox

diox Feb 25, 2014

Member

https://gist.github.com/diox/edf8713381db8fd004c0 fixes it. The culprit was the af.update() call in the form, because it shared the Addon instance with the form (thanks to related models caching introduced in 1.5), the File has a post_save signal that reloads the associated Addon, causing the instance stored on the form to be reseted completely, losing the changes from the form.

I moved the af.update() call after the .save() to fix it.

@diox

diox Feb 25, 2014

Member

https://gist.github.com/diox/edf8713381db8fd004c0 fixes it. The culprit was the af.update() call in the form, because it shared the Addon instance with the form (thanks to related models caching introduced in 1.5), the File has a post_save signal that reloads the associated Addon, causing the instance stored on the form to be reseted completely, losing the changes from the form.

I moved the af.update() call after the .save() to fix it.

This comment has been minimized.

@washort

washort Feb 25, 2014

Contributor

you're a wizard

@washort

washort Feb 25, 2014

Contributor

you're a wizard

mkt/submit/tests/test_forms.py
@@ -186,8 +186,8 @@ def test_slug(self):
form = forms.AppDetailsBasicForm(data, request=self.request,
instance=app)
assert form.is_valid()
+ app.save()

This comment has been minimized.

@diox

diox Feb 24, 2014

Member

why ?

@diox

diox Feb 24, 2014

Member

why ?

This comment has been minimized.

@washort

washort Feb 25, 2014

Contributor

test fails on app.app_slug comparison below otherwise.

@washort

washort Feb 25, 2014

Contributor

test fails on app.app_slug comparison below otherwise.

This comment has been minimized.

@diox

diox Feb 25, 2014

Member

even with the app.reload() that you removed below ?

@diox

diox Feb 25, 2014

Member

even with the app.reload() that you removed below ?

This comment has been minimized.

@washort

washort Feb 25, 2014

Contributor

yes.

@washort

washort Feb 25, 2014

Contributor

yes.

mkt/submit/views.py
if request.POST and all(f.is_valid() for f in forms.itervalues()):
+ addon.save()

This comment has been minimized.

@diox

diox Feb 24, 2014

Member

why ? form_basic.save(addon) below should make that redundant

@diox

diox Feb 24, 2014

Member

why ? form_basic.save(addon) below should make that redundant

This comment has been minimized.

@washort

washort Feb 25, 2014

Contributor

mkt.submit.tests.test_views.TestDetails tests fail if removed.

@washort

washort Feb 25, 2014

Contributor

mkt.submit.tests.test_views.TestDetails tests fail if removed.

requirements/prod.txt
-e git+https://github.com/mozilla/signing-clients@a8bd730b202391c080113d224d223463e03088e9#egg=signing-clients
-e git+https://github.com/mozilla/django-session-csrf@f00ad913c62e139d36078e8a7e07dab65a021386#egg=django-session-csrf
+-e git+https://github.com/jbalogh/django-cache-machine@449861a61bcf096b792039128f044659e3e96eb6#egg=django-cache-machine
+-e git+https://github.com/andymckay/django-quieter-formset.git@904493858e4cf8f442e993ed346ab2c34cf40cd3@#egg=django-quieter-formset

This comment has been minimized.

@diox

diox Feb 24, 2014

Member

I'd prefer to wait for the changes to django-quieter-formset and django-cache-machine are merged before merging this PR. The less requirements we have from personal forks the better.

@diox

diox Feb 24, 2014

Member

I'd prefer to wait for the changes to django-quieter-formset and django-cache-machine are merged before merging this PR. The less requirements we have from personal forks the better.

This comment has been minimized.

@washort

washort Feb 24, 2014

Contributor

OK. django-quieter-formset has a release with my changes, I guess we should ask @jbalogh to do a django-cache-machine release.

@washort

washort Feb 24, 2014

Contributor

OK. django-quieter-formset has a release with my changes, I guess we should ask @jbalogh to do a django-cache-machine release.

This comment has been minimized.

@jbalogh

jbalogh Feb 24, 2014

Contributor

@washort Do you want to be a collab on the cache-machine repo? I don't use it anymore.

@jbalogh

jbalogh Feb 24, 2014

Contributor

@washort Do you want to be a collab on the cache-machine repo? I don't use it anymore.

apps/addons/tests/test_models.py
@@ -2212,7 +2213,8 @@ def test_extract_no_files(self):
def test_extract_not_language_pack(self):
File.objects.create(platform=self.platform_mob, version=self.version,
- filename=self.xpi_path('langpack-localepicker'))
+ filename=self.xpi_path('langpack-localepicker'),
+ status=amo.STATUS_PUBLIC)
assert self.addon.get_localepicker()

This comment has been minimized.

@diox

diox Feb 25, 2014

Member

We should reload the addon here (the File creation might have changed something on it)

@diox

diox Feb 25, 2014

Member

We should reload the addon here (the File creation might have changed something on it)

@@ -79,7 +79,7 @@ def _polite_tmpdir():
# COUNT() caching can't be invalidated, it just expires after x seconds. This
# is just too annoying for tests, so disable it.
-CACHE_COUNT_TIMEOUT = None
+CACHE_COUNT_TIMEOUT = -1

This comment has been minimized.

@diox

diox Feb 25, 2014

Member

Supposing you can import it there, caching.base.NO_CACHE would be better IMHO. According to the code, None should work though:

        if self.timeout == NO_CACHE or TIMEOUT is None:
            return super_count()
        else:
            return cached_with(self, super_count, query_string, TIMEOUT)

Is that change really necessary ?

@diox

diox Feb 25, 2014

Member

Supposing you can import it there, caching.base.NO_CACHE would be better IMHO. According to the code, None should work though:

        if self.timeout == NO_CACHE or TIMEOUT is None:
            return super_count()
        else:
            return cached_with(self, super_count, query_string, TIMEOUT)

Is that change really necessary ?

@diox

This comment has been minimized.

Show comment
Hide comment
@diox

diox Feb 25, 2014

Member

r+wc from me if tests pass after rebasing against master :)

Member

diox commented Feb 25, 2014

r+wc from me if tests pass after rebasing against master :)

@washort

This comment has been minimized.

Show comment
Hide comment
@washort

washort Feb 26, 2014

Contributor

well I merged this and now both CI and -dev deploy are broken with no meaningful errors. reverting until I can get ops help.

Contributor

washort commented Feb 26, 2014

well I merged this and now both CI and -dev deploy are broken with no meaningful errors. reverting until I can get ops help.

Upgrade to Django 1.6.
This reverts commit 62d75b2.

@washort washort closed this Mar 27, 2014

@washort washort deleted the washort:django16 branch Mar 27, 2014

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.