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. On Wed, May 28, 2014 at 5:56 AM, Ahmad Khayyat <akhay...@gmail.com> wrote: > 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_xpermission > 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 mezzanine-users+unsubscr...@googlegroups.com. > For more options, visit https://groups.google.com/d/optout. > -- Stephen McDonald http://jupo.org -- 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-users+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
