Change SiteTreeHints to check for canEdit #1035

Closed
wants to merge 3 commits into
from

Projects

None yet

6 participants

Allowing ability to have
public function canCreate($member = null) {
return MyPage::get()->count() < 1;
}
and still be able to re-order this page in the sitetree.

I have tested moving pages in site tree, with and without the canCreate and canEdit rules, can_be_root and allowed_children statics.

This Fixes #657

@RuthAdele RuthAdele Change SiteTreeHints to check for canEdit
Allowing ability to have
public function canCreate($member = null) {
    return MyPage::get()->count() < 1;
}
and still be able to re-order this page in the sitetree.
a513b47
@wilr wilr and 2 others commented on an outdated diff Jul 1, 2014
code/controllers/CMSMain.php
@@ -386,12 +386,12 @@ public function SiteTreeHints() {
$json = '';
$classes = SiteTree::page_type_classes();
- $cacheCanCreate = array();
- foreach($classes as $class) $cacheCanCreate[$class] = singleton($class)->canCreate();
+ $cacheCanEdit = array();
+ foreach($classes as $class) $cacheCanEdit[$class] = singleton($class)->canEdit();
wilr
wilr Jul 1, 2014 Owner

I think this behaviour is incorrect. canEdit() I assume requires a record rather than being intialized as a singleton (i.e since canEdit would normally check the page access rights). canCreate doesn't normally rely on instance state.

RuthAdele
RuthAdele Jul 2, 2014

Can you give me a resource to look at in order to get this correct? I'm happy to do the work, and I think it's an important issue to fix.

tractorcow
tractorcow Aug 5, 2014 Contributor

Hm, I see the fix you've added, but I'm not sure that using the permissions of the ->first matching instance is a good idea. Pages can have permissions applied to them on a one by one basis. E.g. if you restrict editing to the home page to certain users (type = 'ContentPage') then all other ContentPage types in your tree are unsortable.

@oddnoc oddnoc pushed a commit to oddnoc/silverstripe-cms that referenced this pull request Jul 18, 2014
@RuthAdele RuthAdele + Fred Condo Change SiteTreeHints to check for canEdit
Allowing ability to have
public function canCreate($member = null) {
    return MyPage::get()->count() < 1;
}
and still be able to re-order this page in the sitetree.

Cleaned-up version of [this pull request][1].

[1]: silverstripe#1035
388acf2
@tractorcow tractorcow added the 3.1 label Aug 5, 2014
Contributor

(as commented above, since it probably isn't visible)

Hm, I see the fix you've added, but I'm not sure that using the permissions of the ->first matching instance is a good idea. Pages can have permissions applied to them on a one by one basis. E.g. if you restrict editing to the home page to certain users (type = 'ContentPage') then all other ContentPage types in your tree are unsortable.

Yeah, I know, I guess that's the problem with using canEdit to check for sorting ability. A similar problem already exists with using canCreate (can't move pages that are restricted to one only, i.e. homepage, cartpage, checkoutpage etc)

I suppose we could change the reference array to a page by page basis, instead of class. But I imagine the overhead on this would make thing slower? (can someone confirm?)

Other options could be:

  • remove all permissions on moving pages?
  • make it a permission based on current member rather than page class.
Contributor

Could it be cached by page ID rather than class?

Yes, I thought the same thing. @wilr - can you confirm this won't cause any other issues? And does this sound like a solution to you?

Contributor

You will just have to test it. :)

@RuthAdele RuthAdele Change SiteTreeHints to a page by page basis.
Also checking canEdit permissions rather than canCreate.
f91ed94

This one is going to fail too - can anyone show me how to run the tests without needing to commit here? Or is that the only way to do it?

Contributor

@RuthAdele, you need to install PHPUnit and run them locally.

There is a guide at http://doc.silverstripe.org/framework/en/howto/phpunit-configuration

Thankyou muchly!

@oddnoc oddnoc pushed a commit to oddnoc/silverstripe-cms that referenced this pull request Sep 6, 2014
@RuthAdele RuthAdele + Fred Condo Change SiteTreeHints to check for canEdit
Allowing ability to have
public function canCreate($member = null) {
    return MyPage::get()->count() < 1;
}
and still be able to re-order this page in the sitetree.

Cleaned-up version of [this pull request][1].

[1]: silverstripe#1035
823f9fe

@RuthAdele @tractorcow
What about instead of checking for canCreate or canEdit here, we don't worry about it.

When the user goes to drag an item which is canEdit = false then little notification pops up saying "Forbidden" and the item moves back to where it was in the list anyway.

I don't think it's worth the extra code just to get a red X, and instead we could remove 3 lines of code, modify 1 line (might even run a little faster without the loop over the classes) and we get the desired result.

Any reason we can't solve it with that?

I've created a pull request with my suggestion.
It passes the phpunit tests locally.
#1125

@oddnoc oddnoc pushed a commit to oddnoc/silverstripe-cms that referenced this pull request Jan 22, 2015
@RuthAdele RuthAdele + Fred Condo Change SiteTreeHints to check for canEdit
Allowing ability to have
public function canCreate($member = null) {
    return MyPage::get()->count() < 1;
}
and still be able to re-order this page in the sitetree.

Cleaned-up version of [this pull request][1].

[1]: silverstripe#1035
a86026f
@dhensby dhensby closed this Nov 30, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment