@mbabker Please review

As long as the authorise check works for a root user without explicitly checking it, this seems fine to me.

As long as the authorise check works for a root user without explicitly checking it, this seems fine to me.

Looks like very similar checks to me:
(from JUser::authorise: https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/user/user.php#L366-L377)

// Check for the configuration file failsafe.
$rootUser = JFactory::getConfig()->get('root_user');
// The root_user variable can be a numeric user ID or a username.
if (is_numeric($rootUser) && $this->id > 0 && $this->id == $rootUser)
{
	$this->isRoot = true;
}
elseif ($this->username && $this->username == $rootUser)
{
	$this->isRoot = true;
}

Then we're good.

Except that the unit tests fail since this changes the behavior. ?

I'm lost with that JUser test. @mbabker do you know why that fails?

Rebased the PR. Still don't understand why the unit test fails.

As long as #16411 isn't merged, the same check could be done. This is basically an alternative way of fixing it.

@Bakual #16411 is merged, but the Test Instructions are the same?

I guess you would have to set up a super user account where it hasn't permissions to view all viewlevels. I don't know if that is even possible to do. If not, it's just code review and checking nothing breaks in regards to viewlevels.

But then, I have still no clue how to fix the unit test...

We'd like to help with this one. Where does the issue stand? Is it only the unit test that needs fixing?

@nicksavov As far as I remember it's only the unit test, yes. I'm lost with those.

@Bakual can you update to latest staging, please

Merged staging

Tests still fail and my understanding of testing fails as well.

Just noting here that this would break 3 of the websites that I'm managing. To my customers, this would be a breaking change. I'm pretty sure that I'm not the only one. Besides that, this breaks the Guest viewlevel feature for Superusers. I strongly suggest to not merge this.

Imho, it's indeed a change in behavior. But how it is breaking a site?
To my understanding, a SuperUser is meant to see everything, also guest stuff. And yes that means that a site may look ugly if logged in as SuperUser. Eg you already see articles which are not published as SuperUser.

I have several customers who work with superuser privileges in their daily work. Might not be the best solution, but it is what it is. One customer for example has 4 different usertypes on his site and each usertype has a complete separate set of menu items. Returning all viewlevels means, that instead of 4-6 top level menu items, you get around 20 items displayed. Besides that, each usertype has different sets of modules on a dashboard, again resulting in ~20 modules instead of 5, some of which don't work with superusers.

This is a breaking change.

While the changes makes sense, I agree with @Hackwar

Since we can create a custom menu in the backend it is actually very useful to have separated view levels.
Example:
Super User can have a default backend menu,
And Custom User only "Specific" menu (or many of them. depend from the site set up).

When this patch will be merged then Super User will have all custom menus, that will clutter backend view, and will be no easy way to work around.

it more like a "usability break" ?

@Bakual Regarding unit tests: You see the failure here in drone only if you expand the log file so it's shown completely. The you can see:

There was 1 failure:

1) JUserTest::testGetAuthorisedViewLevels with data set "User42" (42, array(1, 2, 3, 4, 5))
Failed for user 42
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    0 => 1
-    1 => 2
-    2 => 3
-    3 => 4
-    4 => 5
 )

/drone/src/tests/unit/suites/libraries/joomla/user/JUserTest.php:320

The problem could come from this line:

https://github.com/joomla/joomla-cms/blob/staging/tests/unit/suites/libraries/joomla/user/JUserTest.php#L278

I think the expected value null needs to be adjusted to array(1, 2, 3, 4, 5) to reflect the change from this PR.

Does that help somehow?

As discussed in the maintainers chat and also explained here, it doesn't make sense to return all view levels for a super user and would break many workflows. A super user may is allowed to do everything but doesn't mean he/she should see everything. By returning all viewlevels it's not possible to "hide" anything for a super user account which would lead to an unable to use cms.

I'm closing this as discussed by the maintainers team. Thanks all for the comment and the effort.