Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Simple moderation for 2.4 #1463

Merged
merged 47 commits into from

5 participants

@digi604
Collaborator

could you merge from current develop?

@ojii
Collaborator

Thank you for this pull request.

So far I've only skimmed over the code, but it looks like there's still a notion of "approval" in there. Is that just bad naming in the code (from our part)? Because the goal is to get rid of the whole approval mechanism.

@digi604
Collaborator

How should we handle move & delete actions for pages? Should they only be executed on publish like before? I think we should remove this.

@adaptivelogic

It's not finished yet. Added on request - I'm still making sure all parts work as they should with public/draft pages.

We can probably keep delete - it'll be a "request to have this page deleted" noted in the page status until someone hits publish. For move, that's trickier.

The biggest problem is how to manage potentially orphaned child pages, as well as dealing with per-page permissions. Currently, permission lists are only attached to the draft page, so if there's a difference between the draft and public page structure this will be highly confusing if you leave it in this state for a long time. I don't think we want to either make a copy of the lists (ie make you "publish" the changes) or attach them to the public page; both alternatives are very complicated to manage.

My vote is to note it as an issue and ignore it for now. If someone has a problem, tell them to publish the move in order to get it working the way they think it should be. Not too many people use CMS_PERMISSION anyway to judge from comments.

@digi604
Collaborator

Referencing some tickets:

#1151
#1120
#1072

@ojii
Collaborator

The reason perms are attached to the draft page is that the user should never modify the pubic page. They should only be able to make a draft page a public page. That's it.

When you delete a page now without moderator, doesn't it just delete the whole subtree? Shouldn't this stay the same?

adaptivelogic added some commits
@adaptivelogic adaptivelogic Removed all uses of the CMS_MODERATOR flag
Fixed tests to ensure that public and draft pages are used in the proper places.
b76641b
@adaptivelogic adaptivelogic Fixes to the admin interface. 2deb5d7
@digi604
Collaborator

Wow... is looking good. Huge work.

@digi604
Collaborator
@adaptivelogic

Getting pretty massive.
I'm introducing a new Permission ('publish_page') to the model, to allow easy control of who can publish at all. If you have CMS_PERMISSION enabled, you also have to get it enabled globally or for that page. Also had a serious look at permissions and how they're handled; this might need to be refactored but we'll tackle this later.

Basically, we have three levels of access, ignoring anon and super:
1) is_staff=False, has change_page added. Content editors, no access to admin interface other than toolbar
2) is_staff=True. Can add pages, edit content, change template. (gets change_page implicitly)
3) is_staff=True, has publish_page added. Publishers/Moderators, can publish stuff.

With CMS_PERMISSION, you will also need page or global access granted. This is so that if admin decides someone shouldn't have publish permission, they don't need to go chase all over the place to find all the pages where they've been granted this access.

@adaptivelogic

I think that's pretty much it for now. There are some things I broke in the process, like that editing plugins doesn't seem to create PageModerationStates, but they're more or less cosmetic.

Someone should add a revert command to the admin interface and the toolbar, so that you can restore the state of the published version. Currently, it's only used by "manage.py moderation on"

Unpublish refuses to complete if there are child pages that are public. Delete should act the same way - there's no real reason to make deleting content simple. Generally, we should raise errors where for some reason a function call in a page instance can't complete. The key factor is whether the object state is different than expected.

Publish still has the logic to recurse through child pages and publish the ones that are pending on the parent. Not sure we need to keep this. I don't much like the recursing anyway, as it's a good recipe for unintended consequences.

I'm going away for a week, so feel free to either merge this as it is, or fork from my branch and complete the missing details. This branch is getting awfully big and unwieldy, so it's high time to merge it back.

@piquadrat
Collaborator

@adaptivelogic let me just say that you earned yourself a lot of beer or other beverages if/when our paths cross :)

@digi604
Collaborator

Yep, thanks a lot. Will check it out tomorrow.

@digi604
Collaborator

Ok i did some tests... deleting is not possible at the moment or is really bugy

@adaptivelogic

The idea is that non-published pages can be deleted immediately, while published ones result in a delete request.
Seems like that logic used to live in utils.moderator.approve_page, which is now deleted. Put it in Page.publish?

@adaptivelogic

Seems to be working better now, especially for the toolbar editing mode. There are still some oddities when it comes to reverting move requests, but that's a whole bag of snakes all in itself.
I know there's a truckload of changes, but I'd like some review so we can get this merged.

@digi604
Collaborator

From the behavior i want the following:

If i publish: the current page gets published
if i unpublish: the current page and all its children get unpublished.
if i delete a page: I don't think we need to unpublish it first? I mean if you have a delete permission you should be able to do it even if it is published... no?

@digi604
Collaborator

Oh and be sure to check that it behaves correctly in the admin as well.

@adaptivelogic

As I mentioned above, I'm really not too keen on recursive behaviour, especially if it causes loss of content.

For delete, it's a two-step process - mark all the pages you want deleted first, then go to the top-level page and confirm. The last commit puts checks in place to ensure that no pages become orphaned.

For unpublish, it simply checks that there are no published children before letting you proceed. This mirrors the publish action where all your parents have to be published to let you publish. It's simple enough to unpublish that I don't see it as a big concern to have to go through the hierarchy in bottom-up order to remove content. The best way to toggle a large structure from being visible IMO would be to add a view restriction instead.

All of these cases could do with better feedback mechanisms.

@digi604
Collaborator

what do you mean by view restriction?

At the moment its just not very userfriendly and a feedback mechanism would be needed.

@adaptivelogic

I think this takes care of your concerns. I have issues with toggling the publish/unpublish flag on the home page if there was another root, but I couldn't provoke failure in the test. See the tests in publisher.py for how it works.

Also added a feedback mechanism using standard messages.

@digi604
Collaborator

could you merge with develop?

@digi604
Collaborator

Where should i get feedback when i try to delete a page?
If i change a plugin... it is not possible to publish in the list_view... no publish button appears.

@digi604
Collaborator

If i try to delete a page with a child i just can not delete it. If i click on confirm delete: nothing happens.

@adaptivelogic

The delete feedback messages should appear at the top of the change form, using the django messages. You'll get an error if deleting this page would cause child pages of the public page to be orphaned. So either delete them too or move them away and publish, then confirm delete.

cms/admin/pageadmin.py
((23 lines not shown))
page = get_object_or_404(Page, id=page_id)
- if not page.has_moderate_permission(request):
- raise Http404()
+ # ensure user has permissions to publish this page
+ if not page.has_publish_permission(request):
+ return HttpResponseForbidden("Denied")
@ojii Collaborator
ojii added a note

Maybe a little more verbose (and i18n'ed?) message like You do not have permission to publish this page would be more appropriate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cms/admin/pageadmin.py
((36 lines not shown))
- # Django SQLite bug. Does not convert to string the lazy instances
- from django.utils.translation import ugettext as _
- self.message_user(request, _('Page was successfully approved.'))
+ self.message_user(request, ugettext('The page "%s" was successfully published.') % page)
@ojii Collaborator
ojii added a note

I'm a little confused why self.message_user is used here, but on line 830 messages.error is used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cms/admin/pageadmin.py
((79 lines not shown))
return HttpResponseRedirect( path )
+ def confirm_delete(self, request, object_id, *args, **kwargs):
+ """Remove all delete action from page states, requires change permission
+ """
+ page = get_object_or_404(Page, id=object_id)
+ if not page.has_publish_permission(request):
+ return HttpResponseForbidden("Denied")
@ojii Collaborator
ojii added a note

see comment above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cms/admin/pageadmin.py
((79 lines not shown))
return HttpResponseRedirect( path )
+ def confirm_delete(self, request, object_id, *args, **kwargs):
+ """Remove all delete action from page states, requires change permission
+ """
+ page = get_object_or_404(Page, id=object_id)
+ if not page.has_publish_permission(request):
+ return HttpResponseForbidden("Denied")
+
+ page_title = unicode(page)
+ delete_requested = page.pagemoderatorstate_set.get_delete_actions().count()
@ojii Collaborator
ojii added a note

exists instead of count?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cms/admin/pageadmin.py
((29 lines not shown))
- moderator.approve_page(request, page)
+ delete_requested = page.pagemoderatorstate_set.get_delete_actions().count()
@ojii Collaborator
ojii added a note

exists instead of count?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cms/admin/pageadmin.py
((79 lines not shown))
return HttpResponseRedirect( path )
+ def confirm_delete(self, request, object_id, *args, **kwargs):
+ """Remove all delete action from page states, requires change permission
+ """
+ page = get_object_or_404(Page, id=object_id)
+ if not page.has_publish_permission(request):
+ return HttpResponseForbidden("Denied")
+
+ page_title = unicode(page)
+ delete_requested = page.pagemoderatorstate_set.get_delete_actions().count()
+ if not delete_requested:
+ messages.error(request, _('The page "%s" has no delete request.') % page_title)
+ else:
+ try:
+ page.delete_with_public()
+ self.message_user(request, ugettext('The page "%s" was successfully deleted.') % page_title)
@ojii Collaborator
ojii added a note

again as above, I'd rather have consistent messaging (either everything through self.... or messages....

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cms/admin/pageadmin.py
((91 lines not shown))
+ messages.error(request, _('The page "%s" has no delete request.') % page_title)
+ else:
+ try:
+ page.delete_with_public()
+ self.message_user(request, ugettext('The page "%s" was successfully deleted.') % page_title)
+ return HttpResponseRedirect("../../")
+ except PermissionDenied, e:
+ messages.error(request, e.message)
+ return HttpResponseRedirect("../")
+
+ def remove_delete_state(self, request, object_id):
+ """Remove all delete action from page states, requires change permission
+ """
+ page = get_object_or_404(Page, id=object_id)
+ if not self.has_change_permission(request, page):
+ return HttpResponseForbidden("Denied")
@ojii Collaborator
ojii added a note

see above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cms/admin/pageadmin.py
((93 lines not shown))
+ try:
+ page.delete_with_public()
+ self.message_user(request, ugettext('The page "%s" was successfully deleted.') % page_title)
+ return HttpResponseRedirect("../../")
+ except PermissionDenied, e:
+ messages.error(request, e.message)
+ return HttpResponseRedirect("../")
+
+ def remove_delete_state(self, request, object_id):
+ """Remove all delete action from page states, requires change permission
+ """
+ page = get_object_or_404(Page, id=object_id)
+ if not self.has_change_permission(request, page):
+ return HttpResponseForbidden("Denied")
+ page.pagemoderatorstate_set.get_delete_actions().delete()
+ page.save()
@ojii Collaborator
ojii added a note

do we need to save the page after changing the FK? I think not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cms/admin/pageadmin.py
((121 lines not shown))
- if settings.CMS_MODERATOR and page.is_under_moderation():
- # don't perform a delete action, just mark page for deletion
- page.force_moderation_action = PageModeratorState.ACTION_DELETE
- page.moderator_state = Page.MODERATOR_NEED_DELETE_APPROVEMENT
- page.save()
+ # Previously, the standard Django delete view was used for non-published
+ # pages. This was actually more difficult to use than the other workflow,
+ # so it has been disabled to keep things consistent.
+ if False and not page.published:
@ojii Collaborator
ojii added a note

if False..?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ojii ojii commented on the diff
cms/admin/pageadmin.py
@@ -1395,17 +1398,18 @@ def remove_plugin(self, request):
placeholder = plugin.placeholder
page = placeholder.page if placeholder else None
- if page and not page.has_change_permission(request):
- raise Http404
+ if page:
+ if not page.publisher_is_draft:
+ raise Http404
@ojii Collaborator
ojii added a note

exceptions should be instantiated.

This has been fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ojii ojii commented on the diff
cms/api.py
@@ -394,5 +374,9 @@ class FakeRequest(object):
def __init__(self, user):
self.user = user
request = FakeRequest(user)
- moderator.approve_page(request, page)
- return Page.objects.get(pk=page.pk)
+ if not page.has_publish_permission(request):
+ raise PermissionDenied
@ojii Collaborator
ojii added a note

again, please instantiate exceptions.

This has been fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cms/conf/__init__.py
@@ -17,7 +17,7 @@ def patch_settings():
patch_settings.ALREADY_PATCHED = True
if getattr(settings, 'CMS_MODERATOR', False):
- warnings.warn("CMS_MODERATOR will be removed and replaced in django CMS 2.4!", CMSDeprecationWarning)
+ warnings.warn("CMS_MODERATOR has no function in django CMS 2.4!", CMSDeprecationWarning)
@ojii Collaborator
ojii added a note

I think this warning can go away, since it was marked as deprecated in previous releases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cms/forms/utils.py
@@ -16,11 +16,9 @@ def update_site_and_page_choices(lang=None):
lang = lang or translation.get_language()
SITE_CHOICES_KEY = get_site_cache_key(lang)
PAGE_CHOICES_KEY = get_page_cache_key(lang)
- if settings.CMS_MODERATOR:
- title_queryset = Title.objects.filter(page__publisher_is_draft=False)
- else:
- title_queryset = Title.objects.filter(page__publisher_is_draft=True)
- title_queryset = title_queryset.select_related('page', 'page__site').order_by('page__tree_id', 'page__lft', 'page__rght')
+ title_queryset = Title.objects.drafts() \
@ojii Collaborator
ojii added a note

Please use PEP8 style multi line statements without backslashes. either using parenthesis or by using the parenthesis of function calls to start a new line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cms/menu.py
@@ -35,14 +37,13 @@ def get_visible_pages(request, pages, site=None):
for page in pages:
# taken from for_page as multiple at once version
- page_q = Q(page__tree_id=page.tree_id) & (
- Q(page=page)
- | (Q(page__level__lt=page.level) & (Q(grant_on=ACCESS_DESCENDANTS) | Q(grant_on=ACCESS_PAGE_AND_DESCENDANTS)))
- | (Q(page__level=page.level - 1) & (Q(grant_on=ACCESS_CHILDREN) | Q(grant_on=ACCESS_PAGE_AND_CHILDREN)))
- )
+ page_q = Q(page__tree_id=page.tree_id) #& (
+ #Q(page=page)
@ojii Collaborator
ojii added a note

why comment stuff out instead of deleting it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ojii ojii commented on the diff
cms/models/managers.py
@@ -21,9 +21,9 @@ def get_query_set(self):
return PageQuerySet(self.model)
def drafts(self):
- return super(PageManager, self).drafts().exclude(
- publisher_state=self.model.PUBLISHER_STATE_DELETE
- )
+ return super(PageManager, self).drafts()#.exclude(
+ #publisher_state=self.model.PUBLISHER_STATE_DELETE
@ojii Collaborator
ojii added a note

again, just delete, don't comment out. we still have the git history if we need to go back

I was not at all certain removing this was a good idea - was simply trying to eliminate what I saw as unnecessary code. I think we want to see all draft pages no matter their publisher state, but it's still debatable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ojii ojii commented on the diff
cms/models/pagemodel.py
((12 lines not shown))
# check the slugs
page_utils.check_title_slugs(self)
+ def _copy_titles(self, target):
+ """
+ Copy all the titles to a new page.
+ :param target: The page where the new titles should be stored
+ """
+ # TODO: Make this into a "graceful" copy instead of deleting and overwriting
@ojii Collaborator
ojii added a note

agree with this TODO. an update statement (if possible) would be much nicer and faster.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cms/models/pagemodel.py
((7 lines not shown))
old_public.save()
+ #self.publisher_public = None
+ #self.save()
@ojii Collaborator
ojii added a note

see above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cms/models/pagemodel.py
@@ -481,6 +490,63 @@ def publish(self):
return published
+ def unpublish(self):
+ """
+ Removes this page from the public site
+ :returns: True if this page was successfully unpublished
+ """
+ # Publish can only be called on draft pages
+ if not self.publisher_is_draft:
+ raise RuntimeError('The public instance cannot be unpublished. Use draft.')
@ojii Collaborator
ojii added a note

RuntimeError seems to generic. If necessary, create a new exception class in cms.exceptions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
docs/upgrade/2.4.rst
@@ -150,6 +150,14 @@ CMS_FLAT_URLS
Was marked deprecated in 2.3 and has now been removed.
+CMS_MODERATOR
+=============
+Has been deprecated since it is no longer in use. From 2.4 onwards, all pages
@ojii Collaborator
ojii added a note

"Has been removed" is probably more appropriate

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ojii
Collaborator

@adaptivelogic I LOVE this pull request. Please don't feel discouraged by all my nit-picking in inline comments. Will give this branch a test drive and give more feedback. Thank you so much for your work on this.

@FinalAngel This pull request contains a far bit of HTML, CSS and JS changes, mind having a look over them and give some feedback? Thanks!

@ojii
Collaborator

Playing around with it locally I noticed the following weird things:

  • The "Publish" action and "published" checkbox in the changelist view do not work (they seem to do nothing, the row is returned as it was before). This seems to be a UI only issue, as refreshing the changelist will show that it worked.
  • What is the difference between hitting "publish" and checking the "published" flag on the changelist? From what I can see they do the same.
  • Logged in as superuser, I cannot preview unpublished pages with ?preview=1 anymore.
  • Not sure if this is just because of the way I test this, but hitting the "publish" or "revert" buttons on the change view always lead me back to the changelist view. Maybe we should just do the same as "save & continue editing", but this is up for debate.
  • The "Publish draft" button is always displayed, even if there are no changes. Can this be fixed?
  • The "Publish" and "Revert" actions are GET, but they change stuff in the DB. They should really be a POST!

From my very short testing, in general the thing seems to work. So this pull request is definitely on the right track.

@adaptivelogic
  • You may have cached javascript. I changed the change list hook that issues the command, so it works slightly differently now.
  • Publish just "synchronises" the public with the draft. Clicking the checkbox toggles the published status of this page and all subpages - it gets complicated when a page has subpages since the public pages remain in existence but get their published flag changed. Take a peek at the publisher tests.
  • Preview is a little confused; preview=1 tries to view the public page (same as just going to it on the public site), while preview=1&draft=1 should use the draft version. Not sure what to do with the buttons in the list view.
  • Since the publisher state is at present not 100% reliable, I thought it best to always leave the button enabled so you at least can perform the action, whether or not it causes any changes. Caveat publisher, so to speak.
  • Correct, but that would require extra support from javascript to ensure the correct method is used. All the buttons on the top of the change form use GET at present.
@ojii
Collaborator

You don't need JS to do POST requests. We have forms. Put forms instead of buttons in the top left and either do the CSS yourself or let someone like @FinalAngel worry about that :D

I'll double-check if my JS was cached, probably was (silly chrome...)

Ah I was missing the "draft" bit. Never actually used the moderator before, so forgot about that.

For now, always showing the publish button is fine, I just wondered how hard it would be.

@ojii
Collaborator

why? django has method_decorator in django.utils.decorators. So you can do @method_decorator(require_POST).

@ojii
Collaborator

I should've mentioned that in my last comments (sorry I forgot), unlike other places, here you should use the non-lazy ugettext methods.

I decided to replace all the lazy calls with the non-lazy calls in the admin module, since none of them are used to populate a context but are used directly. It makes the code more readable and avoids mistakenly using the lazy call.

@adaptivelogic

I decided not to tinker with the GET/POST issue - it's no worse than it was before, and seems unrelated. If you think it's an issue, open a separate pull request once this one is merged. I'd rather get this beast off my hands than continue to tinker with it.

I made a small change for the preview links. If a page is currently not published, preview=1 will display the public version nonetheless. Also made sure that preview works with multi-language pages. I would like to be able to edit multi-language pages using the toolbar, but I think that requires a separate pull request.

@digi604
Collaborator

could you merge with develop?

@adaptivelogic adaptivelogic Merge remote branch 'upstream/develop' into feature-moderation
Conflicts:
	cms/admin/pageadmin.py
	cms/templatetags/cms_tags.py
	cms/test_utils/cli.py
9a46d5a
@adaptivelogic

So, anything further to be done on this one?

@ojii
Collaborator
@digi604
Collaborator

if i click on publish or published in /en/admin/cms/page/ it seams not to load the new node html... or it is not correct.

This problem seams to occure only in Chrome on dev server... maybe we need to ad a no-cache directive?

Add a @never_cache to this views?

@digi604
Collaborator

closing a tree, reload page, open nodes: Publish end time is missing and the table is too short (/en/admin/cms/page/)

@digi604
Collaborator

Everything else is looking very good.

@digi604
Collaborator

Please merge with develop

@adaptivelogic

Don't ask me why the js fix was needed - attempted some logging but got no clear result. It also appears that change_list.js is a bit noisy with putting things in the global namespace. I don't quite know what parts of that are intentional, but someone with more insight might want to take a look.

@ojii
Collaborator

The js fix looks very sensible to me.

I think the reason change_list.js pollutes the global namespace is that back when it was written, it wasn't written by a frontend engineer. The plus side is that it's in the changelist, so the chances of actually breaking stuff because of the name leak is very low.

At some point, change_list.js will probably need some refactoring, but that's out of scope for this pull request.

@digi604
Collaborator

Deleting Pages:

If i delete a page in the change_list view and then click on the publish icon: nothing happens
The same goes for the backend.
I think it would be nice if we delete it that we see the normal django admin delete view?
Please add the django.contrib.messages app and context processor to cms/test_project/cli.py

@adaptivelogic

Found two issues while testing - moving pages does not mark them as "dirty" in change list, so there's no button to publish the move.
The second issue is that publishing the home page does not retain its home page status in the public list of pages if there's more than one root page. (you may think otherwise, but caching makes this one difficult to see)

I will make sure both of these issues are resolved before we progress with this.

@digi604
Collaborator

I want to publish a 2.4 beta at the end of this week. And i really want this merged :)

@digi604
Collaborator

Ahh now i saw the messages that appear when you try to delete a page in the change_list view.... Could we not make it so that if you click publish it will actually delete the page? instead of going in and confirming the delete request? So this 2 are the same?

@digi604
Collaborator

the messages context processor is missing

Fair enough - I was still running under 1.3 where the messages property is available as part of the auth context processor.

@digi604
Collaborator
  • After a move of a page the draft column is not wide enough and the preview icon is moved into the published column.
  • If i tried to delete the root page of a page tree (without confirming the delete request on the children) i get the following message: "There are pages that would be orphaned. Publish their move requests first." This error message makes no sense.
  • I am still not happy with the delete process. I think the optimal thing would be: I hit delete on a page. The page and all its children get marked RED. If i then hit delete again or publish i get presented with the standard django admin delete view where is see all the objects that will be deleted and i have to confirm.
@adaptivelogic
  • I have no idea what thing is setting the widths. It's not in the templates, so I'm guessing there's some javascript that changes the style explicitly. You would have the same problem whenever a newly loaded fragment has different width for one of the columns.
  • The error message means that you moved some children out of the page tree, but haven't published those move requests. Therefore, the public pages are still children of the page you're trying to remove, and going through with the delete would leave the page tree in an inconsistent state. Therefore, the delete is prevented.
  • I am disinclined to make it easy to delete content. I agree that having to go through all the child pages and clicking delete for each one is more cumbersome, but I think it's a necessary step to ensure that no content gets accidentally deleted.

For the confirm, I actually do agree - that would be optimal, except we would need a customised view to ensure that the public pages also get deleted. The "confirm delete" button on the individual page should also be linked to a confirmation page to keep things consistent. The only case where we might streamline this more would be if there are no child pages. I'm also concerned that the standard Django delete view exposes a fair amount of internal data that the user may not be interested in, such as page placeholders.

I'd also like to see the delete button as a toggle, so that you can remove delete requests from the change list.

All that said, the current functionality is AFAIK working if not 100% smooth, and no worse than before. I'm in favour of dealing with all of these issues as separate pull requests, where we can hash out exactly how things are going to work.

The only real bug at present is that if a move request fails (due to permission problem or because the page was marked to be deleted), you get an error but the node is still moved on the admin page. I tried to figure out how the tree move works, but I'm not that familiar with javascript wizardry.

@digi604 digi604 merged commit c9b232a into divio:develop
@evildmp
Owner

I'm having some trouble with this:

$python manage.py cms moderator on
/home/topdog/test-2012-nov-13/src/django-cms/cms/utils/plugins.py:125: DuplicatePlaceholderWarning: Duplicate placeholder found: `body`
  warnings.warn("Duplicate placeholder found: `%s`" % placeholder, DuplicatePlaceholderWarning)
Traceback (most recent call last):
  File "manage.py", line 10, in <module>
    execute_from_command_line(sys.argv)
  File "/home/topdog/test-2012-nov-13/lib/python2.6/site-packages/django/core/management/__init__.py", line 443, in execute_from_command_line
    utility.execute()
  File "/home/topdog/test-2012-nov-13/lib/python2.6/site-packages/django/core/management/__init__.py", line 382, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/home/topdog/test-2012-nov-13/lib/python2.6/site-packages/django/core/management/base.py", line 196, in run_from_argv
    self.execute(*args, **options.__dict__)
  File "/home/topdog/test-2012-nov-13/lib/python2.6/site-packages/django/core/management/base.py", line 232, in execute
    output = self.handle(*args, **options)
  File "/home/topdog/test-2012-nov-13/src/django-cms/cms/management/commands/subcommands/base.py", line 24, in handle
    handle_command.handle(*args[1:], **options)
  File "/home/topdog/test-2012-nov-13/src/django-cms/cms/management/commands/subcommands/base.py", line 24, in handle
    handle_command.handle(*args[1:], **options)
  File "/home/topdog/test-2012-nov-13/lib/python2.6/site-packages/django/core/management/base.py", line 371, in handle
    return self.handle_noargs(**options)
  File "/home/topdog/test-2012-nov-13/src/django-cms/cms/management/commands/subcommands/moderator.py", line 26, in handle_noargs
    page.publisher_draft.revert()
  File "/home/topdog/test-2012-nov-13/src/django-cms/cms/models/pagemodel.py", line 534, in revert
    public._copy_contents(self)
  File "/home/topdog/test-2012-nov-13/src/django-cms/cms/models/pagemodel.py", line 173, in _copy_contents
    copy_plugins_to(plugins, ph)
  File "/home/topdog/test-2012-nov-13/src/django-cms/cms/utils/copy_plugins.py", line 21, in copy_plugins_to
    new_instance.post_copy(old_plugin, plugins_ziplist)
  File "/home/topdog/test-2012-nov-13/src/django-cms/cms/plugins/text/models.py", line 54, in post_copy
    replace_ids[old.pk] = new.pk
AttributeError: 'NoneType' object has no attribute 'pk'

It happens with more than one database; I'll continue investigating.

@evildmp
Owner

@adaptivelogic #1463 (comment), in case you weren't already notified by GitHub - it seems some people following this were not.

@adaptivelogic

Yeah, no idea what's up with that. Don't think I changed anything in the plugin copy code.

@digi604
Collaborator

Ok we deployed this to a test site and we had many errors. All related to mptt it seams. I think i will remove the commits from this pull request from develop again as it seams to break a lot of things.

Some things we observed and fixed in some commits later on the timeline:

slug title in public will not be updated for children if the publisher_public already exsists.
moving pages or publishing breaks sometimes either the plugin tree or the page tree.

For the first we already have a fix in develop. For the other we haven't found out what the original problem is.

@evildmp
Owner
@digi604
Collaborator

Ok it could be the mptt as well.... but this is quite strange that this was not sooner discovered. Something really strange is happening here. need to investigate further.

@evildmp
Owner

Just to give one example: I have a page, with no children. If I try to delete it, the CMS wants to delete hundreds of other pages that are ancestors of a different page, of which this seems to be some sort of copy.

There's much more madness there. I am not surprised that other things blow up too.

@adaptivelogic
@evildmp
Owner

OK, with a different, much cleaner and simpler database (happy to provide in JSON), python manage.py cms moderator on runs successfully.

However, after running it, there is still some sort of tree-related issue: http://dpaste.com/834260/.

In this case, a Text.objects.get(id=plugin.parent_id) that worked before the moderator command doesn't: DoesNotExist. So it looks to me as if somewhere down the line the plugin tree has been disrupted. More investigating...

@adaptivelogic Why not join us on #django-cms on irc.freenode.net? It's where all the cool kids hang out.

@evildmp
Owner

Pull request #1532 fixes a bug in CMSPlugin.copy_plugin(), and resolves my issues with this, on one database anyway.

I can now migrate, run cms moderator on, and the site works.

Previously, the copies that the moderator command made were corrupted, because of the bug.

@evildmp
Owner

#1564 (delete_orphaned_plugins command) fixes a database that was causing cms moderator on to crash. Maybe cms moderator on should even run the plugin_report() used by delete_orphaned_plugins, and not act until the plugin report finds nothing wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 2, 2012
  1. @adaptivelogic
Commits on Oct 3, 2012
  1. @adaptivelogic
  2. @adaptivelogic
  3. @adaptivelogic

    Adjusted tests to distinguish between draft and public versions of th…

    adaptivelogic authored
    …e page.
    
    Pages no longer save automatically when saving - an explicit call to publish() is needed.
    Removed a bunch of checks of CMS_MODERATOR - the workflow should be the same regardless.
Commits on Oct 4, 2012
  1. @adaptivelogic
  2. @adaptivelogic
  3. @digi604

    fixes templatetags tests

    digi604 authored
Commits on Oct 5, 2012
  1. @adaptivelogic

    Removed all uses of the CMS_MODERATOR flag

    adaptivelogic authored
    Fixed tests to ensure that public and draft pages are used in the proper places.
  2. @adaptivelogic
Commits on Oct 8, 2012
  1. @adaptivelogic

    Fixed permissions for toolbar and admin page actions.

    adaptivelogic authored
    Removed the email approvers form from admin.
Commits on Oct 9, 2012
  1. @adaptivelogic

    Removed PageModerator model and all associated logic.

    adaptivelogic authored
    Changed some logic in the change_form to work better with the new structure.
  2. @adaptivelogic
  3. @adaptivelogic

    Added unpublish method to Page and linked to the change_status view.

    adaptivelogic authored
    Improved tests of permissions and admin interface
  4. @adaptivelogic
  5. @adaptivelogic
  6. @adaptivelogic
  7. @adaptivelogic
Commits on Oct 17, 2012
  1. @adaptivelogic
  2. @adaptivelogic
  3. @adaptivelogic
  4. @adaptivelogic
  5. @adaptivelogic
  6. @adaptivelogic
Commits on Oct 18, 2012
  1. @adaptivelogic
Commits on Oct 19, 2012
  1. @adaptivelogic
Commits on Oct 22, 2012
  1. @adaptivelogic

    Merge branch 'develop' of https://github.com/divio/django-cms into fe…

    adaptivelogic authored
    …ature-moderation
    
    Conflicts:
    	cms/tests/plugins.py
  2. @adaptivelogic
  3. @adaptivelogic

    Documentation.

    adaptivelogic authored
  4. @adaptivelogic
Commits on Oct 26, 2012
  1. @adaptivelogic
Commits on Oct 29, 2012
  1. @adaptivelogic
  2. @adaptivelogic

    Added documentation.

    adaptivelogic authored
  3. @adaptivelogic
Commits on Oct 30, 2012
  1. @adaptivelogic
  2. @adaptivelogic
  3. @adaptivelogic

    Add migrations.

    adaptivelogic authored
Commits on Nov 2, 2012
  1. @adaptivelogic
Commits on Nov 6, 2012
  1. @adaptivelogic

    Merge remote branch 'upstream/develop' into feature-moderation

    adaptivelogic authored
    Conflicts:
    	cms/admin/pageadmin.py
    	cms/templatetags/cms_tags.py
    	cms/test_utils/cli.py
Commits on Nov 9, 2012
  1. @adaptivelogic
  2. @adaptivelogic
Commits on Nov 13, 2012
  1. @adaptivelogic

    Merge remote branch 'upstream/develop' into feature-moderation

    adaptivelogic authored
    Conflicts:
    	cms/tests/page.py
    	cms/utils/moderator.py
  2. @adaptivelogic
  3. @adaptivelogic
  4. @adaptivelogic
  5. @adaptivelogic
Commits on Nov 14, 2012
  1. @adaptivelogic
  2. @adaptivelogic
Something went wrong with that request. Please try again.