django-fluent-pages icon indicating copy to clipboard operation
django-fluent-pages copied to clipboard

Add support for django-reversion.

Open maartendraijer opened this issue 11 years ago • 8 comments

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

maartendraijer avatar Oct 10 '12 12:10 maartendraijer

@vdboor how hard do you think this one?

bashu avatar May 05 '15 07:05 bashu

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?

mrmachine avatar May 18 '15 07:05 mrmachine

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 avatar May 18 '15 11:05 vdboor

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

vdboor avatar Jun 25 '15 14:06 vdboor

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.

jmurty avatar Jun 26 '15 00:06 jmurty

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/ :)

vdboor avatar Aug 04 '15 13:08 vdboor

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 https://github.com/etianen/django-reversion/pull/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 avatar Aug 12 '15 06:08 jmurty

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.

jmurty avatar Aug 12 '15 07:08 jmurty