Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for django-reversion. #11

Open
maartendraijer opened this issue Oct 10, 2012 · 8 comments
Open

Add support for django-reversion. #11

maartendraijer opened this issue Oct 10, 2012 · 8 comments

Comments

@maartendraijer
Copy link
Contributor

So that a page can be reverted to a previous version.

@bashu
Copy link
Member

bashu commented May 5, 2015

@vdboor how hard do you think this one?

@mrmachine
Copy link
Contributor

Reversion support would be great. Any tips on the roadblocks or how they might be worked around? I'd like to look into this as well. Alternatively, any other options for versioning that are compatible with fluent?

@vdboor
Copy link
Contributor

vdboor commented May 18, 2015

I'm not sure how hard this is, I didn't work with reversion before.
There are a few things you'll have to overcome:

  • Take the base model + all translations in a single "revision".
  • Handle the custom content on the page, especially when combining this with fluent-contents

For the content items that fluent-contents provides, you could implement a versioning system within fluent-contents itself. Each ContentItem is linked to it's parent and we already implement multilingual support there by filtering on the 'language_code' field. It could be trivial to filter by "version" too if "0" is always the current version.

I've just discovered django-widgy last week (somewhat comparable to fluent-contents, we could even make a fluentcms-widgy package to add a "WidgyPage"). Sadly they only add versioning for the content, and not the whole page. Ideally I'd love to see the whole page getting revisioned/versioned.

@vdboor
Copy link
Contributor

vdboor commented Jun 25, 2015

I noticed that Aldryn is actively using django-parler for multilingual support, and they have written a version adapter for it. It's worth looking at, and if anyone would be able to port that into a module for parler, I'd be very happy!

See:
https://github.com/aldryn/aldryn-reversion/blob/master/aldryn_reversion/admin.py
https://github.com/aldryn/aldryn-reversion/blob/master/aldryn_reversion/core.py

@jmurty
Copy link
Contributor

jmurty commented Jun 26, 2015

FYI we are working on adding support for versioning of Fluent and Flat pages using django-reversion in this fork: https://github.com/ixc/django-fluent-pages/tree/reversion_support

The fork is partly based on https://github.com/aldryn/aldryn-reversion though without the support for plugins.

It is not quite mature enough to become a pull request just yet, but we are getting there and plan to help towards adding official versioning support if possible. In our environment basic versioning, revert, and restore of single-language Flat/Fluent pages is working. See the commit comments on the "reversion_support" branch for a rough guide to getting started.

While this branch is still under development any testing by, or feedback from, others in the community would be great!

@vdboor Maybe we should open a PR despite the code being very much in-flux, to have a better place to discuss? Whatever you prefer.

@vdboor
Copy link
Contributor

vdboor commented Aug 4, 2015

Wow, thanks a lot for the work so far! This will require some digging into the solution.
Interesting you took advantage of the FLUENT_PAGES_PARENT_ADMIN_MIXIN setting.
Sorry for not paying attention to the GitHub tickets lately...

Based on looking through the code, there are a few things I wonder:

  • Does this code already work with the latest django-reversion?
  • Could it be made in such way that FLUENT_PAGES_PARENT_ADMIN_MIXIN doesn't have to be used? For example, automatically mix in the reversion support when reversion is available? Or would that cause too many issues?
  • Is there ever a reason to disable reversion support for pages when the rest of your project runs on reversion?
  • The ReversionRealtimeSignalProcessor seems like a workaround. Can this be fixed for real, or should this be upstreamed to django-parler?
  • If this feature doesn't need to be in INSTALLED_APPS, it would make sense to upstream it to django-fluent-utils, so it can be integrated into django-fluent-blogs and django-fluent-faq too.
  • What is the relation with django-model-publisher? It seems like a nice replacement of the default status field.

I'm fine with keeping the discussion here, it's a 50/50 choice imho.

btw, if you have some sites running on django-fluent, I'd love to add those to https://django-fluent.org/showcase/ :)

@jmurty
Copy link
Contributor

jmurty commented Aug 12, 2015

Hi @vdboor thanks for the comments and questions. Sorry for the late reply, the code branch has been in a lot of flux and I wanted to get it (somewhat) more stable before replying.

Based on your feedback I have updated the integration to automatically support versioning by modifying the relevant admin classes to extend the integration versions if reversion is installed and enabled. I tried to do this in minimally intrusive way, and it works but is fairly hacky. For example

  • Does this code already work with the latest django-reversion?
    It uses the latest django-reversion except for one outstanding pending fix for polymorphic page links, see Add content type ID to links on recover list page for polymorphic items etianen/django-reversion#438 The maintainer of django-reversion Dave Hall has been super-helpful in merging and updating the library to work better in complex situations like for fluent pages
  • Automatically mix in reversion support and avoid requiring the FLUENT_PAGES_PARENT_ADMIN_MIXIN setting
    Done
  • Is there ever a reason to disable reversion support for pages when the rest of your project runs on reversion?
    Maybe not. I can imagine situations where some parts of a site use reversion but you don't want fluent pages to do so, though it's unlikely. I would like to keep the FLUENT_PAGES_DISABLE_REVERSION optional setting for now at least, as it provides a quick way to disable the versioning support to see whether or not it is causing a specific issue
  • The ReversionRealtimeSignalProcessor seems like a workaround. Can this be fixed for real, or should this be upstreamed to django-parler?
    The term "workaround" is too kind, it's more of a hack. It would be good to fix it for real, though I'm not sure of the best way to do this. The problem may actually have been solved by recent changes in django-reversion to do strict revert/restore operations – I haven't had a chance to re-test
  • If this feature doesn't need to be in INSTALLED_APPS, it would make sense to upstream it to django-fluent-utils, so it can be integrated into django-fluent-blogs and django-fluent-faq too.
    That sounds reasonable. It isn't something I will have time to take on in the near-term, but once it is working well it will hopefully be possible to relocate the bulk of the code.
  • What is the relation with django-model-publisher? It seems like a nice replacement of the default status field.
    We are actually using the integration along with django-model-publisher though with a lot of code customisations. That could make for another good integration project in the future.
  • btw, if you have some sites running on django-fluent, I'd love to add those to https://django-fluent.org/showcase/ :)
    Heh, nothing that's publicly visible just yet but that sounds like a good idea for when the site eventually goes live.

@jmurty
Copy link
Contributor

jmurty commented Aug 12, 2015

I should note that with the latest commit to our integration branch we are back to using the official 1.9.3 release of django-reversion, instead of our forked and patched version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants