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()<https://bitbucket.org/stephenmcd/mezzanine/src/389882485a3e731adefaf8e076f4febdc90038f0/mezzanine/pages/admin.py?at=default#cl-143>
).
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?!
--
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 [email protected].
For more options, visit https://groups.google.com/d/optout.
