Proposal: page_move signals

[Ahmad Khayyat]

Objective

Support arbitrary rules in the page tree, e.g. some content types have to always by under some other types, or specific objects, and having sections of the page tree that are exclusive to some specific content type(s).

Description

The admin_page_ordering view of the pages app is responsible for performing the reordering of pages as a result of a drag-and-drop of pages in the page change list (tree) in the admin.

It would be very useful to expose the IDs of the page being moved and its new parent to some other user code, so that it can decide whether to go ahead with the move, or abort it. This can be done by having the view above send a pre_page_move signal with those two arguments. A receiver can abort the change by setting its return value.

What I do now is that I:

1. override the url of the view above, and coerce it to go to a custom view instead

2. duplicate some of the code in the view above in my custom view to obtain the IDs of the page being moved and the new parent

3. perform some logic on these two IDs to decide whether to call the original view to perform the move, or not to call it to abort the move
4. in case the move is aborted, the page may need to be reloaded to put the moved page back where it was in the admin page tree. This can be done very neatly and consistently using django messages, but that requires a tiny modification of Mezzanine’s page_tree.js. This is a solved problem.

If this proposal is accepted, all is needed, instead, is to subscribe to the signal, perform the logic on the provided argument, and return the decision, maybe along with a message explaining the problem, if possible (is it?).

What do you think?

​

[Ahmad Khayyat]

T​​

o complete the thought, here is what I’m doing now with what I described earlier:

diff --git a/mezzanine/pages/static/mezzanine/js/admin/page_tree.js b/mezzanine/pages/static/mezzani
--- a/mezzanine/pages/static/mezzanine/js/admin/page_tree.js
+++ b/mezzanine/pages/static/mezzanine/js/admin/page_tree.js
@@ -91,7 +91,10 @@

         $.post(window.__page_ordering_url, args, function(data) {
             if (String(data).substr(0, 2) !== "ok") {
-                alert("Error occured: " + data + "\nOrdering wasn't updated.");
+                location.reload();
+            }
+            else {
+                $(".messagelist").remove();
             }
         });

This will result in the page tree being reverted if the page move is aborted. Also, a django-messages error message is shown (done elsewhere right now, but ideally should be done in the admin_page_ordering view. Upon a successful page move, the message is reset.

​

[Josh Cartmell]

I think this sounds really cool, I've definitely wanted to do things along these lines before.

--
You received this message because you are subscribed to the Google Groups "Mezzanine Users" group.
To unsubscribe from this group and stop receiving emails from it, send an email to mezzanine-use...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Stephen McDonald]

Sounds like a good idea, go for it.

--
Stephen McDonald
http://jupo.org

[Ahmad Khayyat]

Here we go:
https://bitbucket.org/stephenmcd/mezzanine/pull-request/62/

Create and send pre_page_move and post_page_move signals.
Any value returned by any receiver of the pre_page_move signal will abort the move. The value is considered an error message, and will be displayed. The first value received aborts further processing.

Needs to be documented.

Receivers can subscribe to moves of specific content types using the sender argument.

Example:

from mezzanine.pages.signals import pre_page_move

@receiver(pre_page_move, sender=MyPage)
def my_move_constraints(sender, page, new_parent, **kwargs):

    ...

​

[Ahmad Khayyat]

​Any comments?
Any additional information required?

Here is an example that uses this feature to disallow pages of type MyPage to be top-level pages:

from mezzanine.pages.signals import pre_page_move

@receiver(pre_page_move, sender=MyPage)
def my_move_constraints(sender, page, new_parent, **kwargs):

    if new_parent is None:
        return "Pages of type MyPage cannot be top-level pages"

​If a user drags a MyPage to a top-level position in the page tree in Mezzanine admin, the page will be moved back to its original position, and an error message will be displayed using Django messages. The text of the error message is the returned string: “Pages of type MyPage cannot be top-level pages”. A subsequent valid move will remove the error message.​

​

[Stephen McDonald]

Seems incomplete for the use-case - couldn't the user create a new MyPage instance as a top-level page?

--

You received this message because you are subscribed to the Google Groups "Mezzanine Users" group.
To unsubscribe from this group and stop receiving emails from it, send an email to mezzanine-use...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Ahmad Khayyat]

Right. Yes, they can. But that would be handled by the save method of the MyPage model (this is what I do in my app). I don't know of a way to handle it in the page tree.

--
You received this message because you are subscribed to a topic in the Google Groups "Mezzanine Users" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/mezzanine-users/bIxUJkOIdUM/unsubscribe.
To unsubscribe from this group and all its topics, send an email to mezzanine-use...@googlegroups.com.

[Ahmad Khayyat]

Put another way: creating an instance is not a page move.

[Stephen McDonald]

Sure - but if that's the intended use case then perhaps we need a better approach.

I tried to implement something like this with page permissions, but I don't think they cover the case of moving a page:

http://mezzanine.jupo.org/docs/content-architecture.html#page-permissions

There really should be one way to implement these rules.

[Ahmad Khayyat]

Sure, but one issue is that violating the rule is expected to result in different reactions when creating vs. when moving a page.

If a rule is violated in a page move, the page goes back to its original position.

If a rule is violated while creating a page, there should be a way to determine a default position of the page.

How about this, along the lines of page permission methods:

can_move(self, new_parent)

Should return a Boolean and an alternative new_parent and error message if the Boolean is False?

It would be called by Page.save() and by admin_page_ordering.

The problem with creating a page is that you cannot modify the page order in the change form, so using validation won’t work. Disallowing the save is not really desirable because the user would have to recreate the page and reenter all the data.

But still, what should happen if the rule disallows top-level pages and an alternative parent is not provided?

---

Back to the original pull request, note that the current situation is that I can enforce rules for page position during page creation by overriding the save method, but there is no mechanism for applying any rules to page moves. The purpose of the pull request is just to introduce that hook.

​

[Stephen McDonald]

On Mon, May 26, 2014 at 11:52 PM, Ahmad Khayyat <akha...@gmail.com> wrote:

Sure, but one issue is that violating the rule is expected to result in different reactions when creating vs. when moving a page.

If a rule is violated in a page move, the page goes back to its original position.

If a rule is violated while creating a page, there should be a way to determine a default position of the page.

How about this, along the lines of page permission methods:

can_move(self, new_parent)

That sounds spot on - and I think it'd work well with most of the work you've done. We'd just replace the signals with a call to can_move and all the error propagation stuff should work the same way you've got so far.

Since we have the other page permission methods, and no signals defined throughout Mezzanine, this really makes sense in terms of consistency. Should make documenting easier too since everything's falls under the same umbrella.

[Ahmad Khayyat]

Having spent some time trying to implement this can_move permission, I see a few issues with it:

1. It has to be called by Mezzanine’s page-move and page-create code, which is not straightforward (more on this in point 3 below), to supposedly provide a common interface in both scenarios. However, the user will most likely want different behaviors in the create and move cases, because an illegal move can be reverted to the initial position, but an illegal create position must by corrected by specifying an alternative new parent. So, the user implementation of can_move ends up being split into code that handles an illegal create position, and code that handles an illegal move destination — back where we started, only deceivingly combined.

2. To amend the previous point, the return type is different for the two scenarios. A negative can_move call is expected to return an error message in the case of an illegal move, to display it as an explanation for reverting the page position. In the case of an illegal create, however, it is expected to return an alternative new parent so the newly created page can land somewhere. It would be perfect to prevent their creation under an illegal parent, but can_move is an instance method that requires the object to exists.

3. As hinted in point 1 above, calling can_move from Mezzanine’s page-create code is not straightforward. AFAICT, the best place is the save_model method in pages.admin. admin.save_model has access to the request, while Model.save does not. To get to the content model instance (to call can_move), the page must first be saved, and for it to be saved, the parent must be set first, which suggests that the initial parent must be set, the page saved, then can_move is run, and possibly the new parent set depending on the return values of can_move, which might result in re-updating the order/position of relevant pages, again (see pages.admin.save_model()).

4. can_move will have a different interface from the other can_x permission methods, in terms of both inputs and output. For input, it needs the new parent in addition to the common request input. The new parent can be attached to request, but I don’t think that’s a good idea (implicit). For output, a simple Boolean return value, as is the case for the other methods, is not sufficient. Worse, the page-create code can use different return values (new parent) than what the page-move code can (error message).

I guess most of these issues are stylistic, non-deal-breakers, but I thought I’d point them out before I get any deeper.

What do you think, list?!

​

[Stephen McDonald]

Thanks for putting these points together - really well thought out.

I think it's OK that can_move has a slightly different interface (exceptions vs boolean returns), so long as we document that. 

I think calling can_move when a page is created is also OK - the fact it returns an error messages perfectly suits using Django's messages in the admin to return an error message. You're right it's fiddly finding the right spot to call this code. Perhaps it belongs in the admin model form validation somehow? As I understand it would need to execute *before* an instance is saved, but you hinted that this is hard/undoable with the requirement of accessing the can_move method on the underlying content type (page subclass) instance - perhaps there's a way to simulate this within the form validation, by creating an unsaved instance of the content type and populating it with the form's field data. Not sure, just throwing some ideas around. 

To be honest if it turns out to be too difficult, we could forgo calling can_move on page creation and just document this shortcoming. At least we'll have a semi consistent API for defining these rules.

--

You received this message because you are subscribed to the Google Groups "Mezzanine Users" group.
To unsubscribe from this group and stop receiving emails from it, send an email to mezzanine-use...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Ahmad Khayyat]

OK. I took another stab at it:
https://bitbucket.org/stephenmcd/mezzanine/pull-request/63/can_move-permission/

Pull request description:

Implement a new can_move dynamic page permission.

Content types can override can_move() to control whether a given page
move in the page tree is permitted. can_move() should return a
3-tuple: the permission (Boolean), an error message (String), and an
alternative new parent (Page). The last two elements are used only
when the permission is False.

Currently, the error message is used in page moves only to explain why the page move is reverted. Also, the alternative new parent is used in page creation only to insert the new page in a legal position.

During page creation, can_move() is called at the end, after all other save-related operation

​s​

are performed, including setting the slugs based on the original parent. This may not be optimal, as it may result in an additional save, but it is a clean and simple way. Trying to call can_move() any earlier is tricky, because the specific content type that defines can_move() is attached to the Page instance only after it has been saved.

Calling can_move() during page creation is separated into its own commit (5d2e613).

​

[Stephen McDonald]

Awesome.

What do you think about having the api for can_move simply either raise an exception or not? If it does it can contain the error message. I'd guess there's no need to define the alternative parent - it should always be the previous parent prior to moving.

--

You received this message because you are subscribed to the Google Groups "Mezzanine Users" group.
To unsubscribe from this group and stop receiving emails from it, send an email to mezzanine-use...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Ahmad Khayyat]

On Sat, May 31, 2014 at 3:32 AM, Stephen McDonald <st...@jupo.org> wrote:

 

I'd guess there's no need to define the alternative parent - it should always be the previous parent prior to moving.

On Sat, May 31, 2014 at 8:59 AM, Ahmad Khayyat <akha...@gmail.com> wrote:

Also, the alternative new parent is used in page creation only to insert the new page in a legal position.

​The alternative parent is used when creating a new page in an illegal position. The alternative parent argument can be stored in the request, but I thought an explicit separate argument is more appropriate, as it is /required/ for page creation. There is no other way really to determine the parent of a new page if the one it was created under is illegal.​

[Ahmad Khayyat]

Oops.. I mixed two things.. The new parent passed to can_move can be stored in the request argument, but it probably shouldn't.

The new parent returned by can_move, which was the subject of your suggestion, is really required in page creation to fall back to in case the original parent is illegal.

- If a page move is illegal, the error message is needed (and can be enclosed in an exception as you suggested).

- If a page is created in an illegal position, the new parent is needed so the page gets created somewhere, instead of being discarded, and the user having to re-enter its data. I don't see how else the new page can be placed, except for a brute-force-lookup of a legal position by examining every existing page as a potential parent.

[Stephen McDonald]

On Sat, May 31, 2014 at 11:07 AM, Ahmad Khayyat <akha...@gmail.com> wrote:

Oops.. I mixed two things.. The new parent passed to can_move can be stored in the request argument, but it probably shouldn't.

The new parent returned by can_move, which was the subject of your suggestion, is really required in page creation to fall back to in case the original parent is illegal.

- If a page move is illegal, the error message is needed (and can be enclosed in an exception as you suggested).

- If a page is created in an illegal position, the new parent is needed so the page gets created somewhere, instead of being discarded, and the user having to re-enter its data. I don't see how else the new page can be placed, except for a brute-force-lookup of a legal position by examining every existing page as a potential parent.

If can_move was validated in the admin's model form, and raised a form validation error, wouldn't the data entered be retained in the browser without having to ever save anything to the db? Requiring the developer to write rules that chooses a parent that the user didn't even select sounds really convoluted. 

 

On Sat, May 31, 2014 at 3:40 AM, Ahmad Khayyat <akha...@gmail.com> wrote:

On Sat, May 31, 2014 at 3:32 AM, Stephen McDonald <st...@jupo.org> wrote:

 

I'd guess there's no need to define the alternative parent - it should always be the previous parent prior to moving.

On Sat, May 31, 2014 at 8:59 AM, Ahmad Khayyat <akha...@gmail.com> wrote:

Also, the alternative new parent is used in page creation only to insert the new page in a legal position.

​The alternative parent is used when creating a new page in an illegal position. The alternative parent argument can be stored in the request, but I thought an explicit separate argument is more appropriate, as it is /required/ for page creation. There is no other way really to determine the parent of a new page if the one it was created under is illegal.​

--
You received this message because you are subscribed to the Google Groups "Mezzanine Users" group.
To unsubscribe from this group and stop receiving emails from it, send an email to mezzanine-use...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Ahmad Khayyat]

On Sun, Jun 1, 2014 at 9:11 AM, Stephen McDonald <st...@jupo.org> wrote:

If can_move was validated in the admin's model form, and raised a form validation error, wouldn't the data entered be retained in the browser without having to ever save anything to the db?

​Yes, but how would the user fix the validation error and change the parent causing it?​​

[Stephen McDonald]

Good point! So that still leaves us with something that I personally think is really awkward. Sorry for pushing back hard on this so far, it just hasn't felt right to me and I was hoping we'd work out something clean, but it hasn't gotten there yet.

Perhaps including this in the save of objects isn't right and it should just be something implemented when actually moving pages.

Does anyone else have any input? 

--
You received this message because you are subscribed to the Google Groups "Mezzanine Users" group.
To unsubscribe from this group and stop receiving emails from it, send an email to mezzanine-use...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.