Fix support for reversion >=1.9 #4883
Conversation
yakky
added
status: work in progress
status: ready for review
and removed
status: work in progress
labels
yakky
changed the title
|
@FinalAngel @mkoistinen this is complete, but it requires extensive testing as many moving parts have changed and 1.10 has a very different admin integration logic |
yakky
force-pushed
the
feature/fix_reversion
branch
from
e99dea4
to
019a042
Compare
| @@ -1,6 +1,26 @@ | |||
| # -*- coding: utf-8 -*- | |||
| import reversion | |||
| from reversion.revisions import RegistrationError, VersionAdapter | |||
|
|
|||
There was a problem hiding this comment.
It looks like you're importing the same stuff even on ImportError, it's best to explicitly target the imports that will fail and add some comment as to why they would fail ie. "Reversion >= 1.x compat".
Also the revision manager assignments can easily be outside of the ImportError try/except and placed on their own try/except that catches the AttributeError, more specifically only the revision_manager seems to change location
There was a problem hiding this comment.
Well, I could easily change this in a simple version checking.
Having a simple try / except will make easier to remove the legacy imports in 3.3 (as we can use reversion 1.10 for all the supported Django versions) and a single section makes the code more readable. I'll happily add a comment to clarify the reason of the code style
yakky
force-pushed
the
feature/fix_reversion
branch
from
32b29fe
to
5d94bcd
Compare
| @@ -1,9 +1,6 @@ | |||
| -r requirements_base.txt | |||
| Django>=1.8,<1.9 | |||
| django-reversion>=1.8,<1.9 | |||
| django-classy-tags>=0.6.1 | |||
| django-reversion==1.10.0 | |||
There was a problem hiding this comment.
Shouldn't this be ...>=1.10,<1.11?
|
Good work, @yakky I have personally tested this a lot (in a test project, not a live project) and seems to work well. I've made a couple of comments about the requirements, but otherwise LGTM. see note below |
|
OK, I've now tested this in existing projects. Restoring revisions created recently (since the new django-revision was installed?), seem to work OK. Restoring older revisions fails with a Database Integrity issue as show here: https://gist.github.com/mkoistinen/140ff90d2ea077629588. It is possible (but rather unlikely) that this could be related to some previous corruption in the DB. Also, after restoring even a "working" revision, when I attempt to publish them, the AJAX request that occurs as a result of pressing "Publish" returns this: https://gist.github.com/mkoistinen/5aae3398498316659f56. I've been able to reproduce this errors on-demand using a new project (using djangocms-installer). Finally, pressing the "View published" on a restored page gives me: https://gist.github.com/mkoistinen/37d4f775c4e142783fb8. This is also repeatable. =( |
|
@mkoistinen I'll have a look at this shortly |
|
@mkoistinen I've been able to reproduce both issues and they are hopefully fixed |
|
Awesome! My tests look good too. I'll merge this, thanks @yakky! |
Fix support for reversion >=1.9
Slowly approaching to full compatibility.
Reversion logic has changed a lot: 1.8 & 1.9+ compatibiliy layer added, to be dropped in 3.3 when we drop Django 1.6/1.7 and can thus focus on reversion 1.10+
Fix #4784
This is a blocker for #4812 (which is complete as far as Django 1.9 is involved)