Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

make Settings of Personal and Admin into own views #40544

Closed
wants to merge 4 commits into from
Closed

make Settings of Personal and Admin into own views #40544

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1b774d7
make Settings of Personal and Admin into own views
privatemaker Sep 20, 2023
f2f8a97
Merge branch 'master' into feat/personal-or-admin-settings
privatemaker Sep 21, 2023
2b488d8
Merge branch 'nextcloud:master' into feat/personal-or-admin-settings
privatemaker Sep 21, 2023
bb2353a
add test files for apps/settings changes #40490
privatemaker Sep 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion apps/settings/lib/Controller/AdminSettingsController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
* @author Lukas Reschke <lukas@statuscode.ch>
* @author Robin Appelman <robin@icewind.nl>
* @author Kate Döen <kate.doeen@nextcloud.com>
* @author Private Maker <privatemaker@posteo.net>
*
* @license GNU AGPL version 3 or any later version
*
Expand Down Expand Up @@ -68,7 +69,7 @@ public function __construct(
* settings for the given section. The user will be gretted by an error message.
*/
public function index(string $section): TemplateResponse {
return $this->getIndexResponse('admin', $section);
return $this->getAdminResponse($section);
}

/**
Expand Down
46 changes: 30 additions & 16 deletions apps/settings/lib/Controller/CommonSettingsTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
* @author Morris Jobke <hey@morrisjobke.de>
* @author Robin Appelman <robin@icewind.nl>
* @author Roeland Jago Douma <roeland@famdouma.nl>
* @author Private Maker <privatemaker@posteo.net>
*
* @license GNU AGPL version 3 or any later version
*
Expand Down Expand Up @@ -133,28 +134,41 @@ private function formatSettings(array $settings): array {
return ['content' => $html];
}

private function getIndexResponse(string $type, string $section): TemplateResponse {
if ($type === 'personal') {
if ($section === 'theming') {
$this->navigationManager->setActiveEntry('accessibility_settings');
} else {
$this->navigationManager->setActiveEntry('settings');
}
} elseif ($type === 'admin') {
$this->navigationManager->setActiveEntry('admin_settings');
private function getPersonalResponse(string $section): TemplateResponse {
$templateParams = [];
$templateParams = array_merge($templateParams, $this->getNavigationParameters('personal', $section));
$templateParams = array_merge($templateParams, $this->getSettings($section));
$activeSection = $this->settingsManager->getSection('personal', $section);

if ($activeSection) {
$templateParams['pageTitle'] = $activeSection->getName();
$templateParams['activeSectionId'] = $activeSection->getID();
}

$templateParams = [];
$templateParams = array_merge($templateParams, $this->getNavigationParameters($type, $section));
if ($section === 'theming') {
$this->navigationManager->setActiveEntry('accessibility_settings');
} else {
$this->navigationManager->setActiveEntry('settings');
}

return new TemplateResponse('settings', 'settings/personal', $templateParams);
}

private function getAdminResponse(string $section): TemplateResponse {
$templateParams = [];
$templateParams = array_merge($templateParams, $this->getNavigationParameters('admin', $section));
$templateParams = array_merge($templateParams, $this->getSettings($section));
$activeSection = $this->settingsManager->getSection($type, $section);
if ($activeSection) {
$activeSection = $this->settingsManager->getSection('admin', $section);

if ($activeSection) {
$templateParams['pageTitle'] = $activeSection->getName();
$templateParams['activeSectionId'] = $activeSection->getID();
}
}

return new TemplateResponse('settings', 'settings/frame', $templateParams);
}
$this->navigationManager->setActiveEntry('admin_settings');

return new TemplateResponse('settings', 'settings/admin', $templateParams);
}

abstract protected function getSettings($section);
}
3 changes: 2 additions & 1 deletion apps/settings/lib/Controller/PersonalSettingsController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
* @author Joas Schilling <coding@schilljs.com>
* @author Robin Appelman <robin@icewind.nl>
* @author Kate Döen <kate.doeen@nextcloud.com>
* @author Private Maker <privatemaker@posteo.net>
*
* @license GNU AGPL version 3 or any later version
*
Expand Down Expand Up @@ -64,7 +65,7 @@ public function __construct(
* @NoSubAdminRequired
*/
public function index(string $section): TemplateResponse {
return $this->getIndexResponse('personal', $section);
return $this->getPersonalResponse($section);
}

/**
Expand Down
56 changes: 56 additions & 0 deletions apps/settings/templates/settings/admin.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
<?php
/**
* @copyright Copyright (c) 2016 Arthur Schiwon <blizzz@arthur-schiwon.de>
*
* @author Arthur Schiwon <blizzz@arthur-schiwon.de>
* @author Private Maker <privatemaker@posteo.net>
*
* @license GNU AGPL version 3 or any later version
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License as
* published by the Free Software Foundation, either version 3 of the
* License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*
*/

style('settings', 'settings');
script('settings', 'settings');
\OCP\Util::addScript('settings', 'legacy-admin');
script('core', 'setupchecks');
script('files', 'jquery.fileupload');
?>

<div id="app-navigation" role="navigation">
<ul tabindex="0">
<li class="app-navigation-caption"><?php p($l->t('Administration')); ?></li>
<?php foreach ($_['forms']['admin'] as $form):
if (isset($form['anchor'])):
$anchor = \OC::$server->getURLGenerator()->linkToRoute('settings.AdminSettings.index', ['section' => $form['anchor']]);
github-advanced-security[bot] marked this conversation as resolved.
Fixed Show resolved Hide resolved
$class = 'nav-icon-' . $form['anchor'];
$sectionName = $form['section-name']; ?>
<li <?php print_unescaped($form['active'] ? ' class="active"' : ''); ?> data-section-id="<?php print_unescaped($form['anchor']); ?>" data-section-type="admin">
<a href="<?php p($anchor); ?>"<?php print_unescaped($form['active'] ? ' aria-current="page"' : ''); ?>>
<?php if (!empty($form['icon'])) { ?>
<img alt="" src="<?php print_unescaped($form['icon']); ?>">
<span><?php p($form['section-name']); ?></span>
<?php } else { ?>
<span class="no-icon"><?php p($form['section-name']); ?></span>
<?php } ?>
</a>
</li>
<?php endif ?>
<?php endforeach ?>
</ul>
</div>
<div id="app-content" tabindex="0" data-active-section-id="<?php print_unescaped($_['activeSectionId']) ?>">
<?php print_unescaped($_['content']); ?>
</div>
43 changes: 6 additions & 37 deletions apps/settings/templates/settings/frame.php → .../settings/templates/settings/personal.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
* @copyright Copyright (c) 2016 Arthur Schiwon <blizzz@arthur-schiwon.de>
*
* @author Arthur Schiwon <blizzz@arthur-schiwon.de>
* @author Private Maker <privatemaker@posteo.net>
*
* @license GNU AGPL version 3 or any later version
*
Expand All @@ -26,17 +27,13 @@
\OCP\Util::addScript('settings', 'legacy-admin');
script('core', 'setupchecks');
script('files', 'jquery.fileupload');

?>

<div id="app-navigation" role="navigation">
<ul tabindex="0">
<?php if (!empty($_['forms']['admin'])) { ?>
<li class="app-navigation-caption"><?php p($l->t('Personal')); ?></li>
<?php
}
foreach ($_['forms']['personal'] as $form) {
if (isset($form['anchor'])) {
<li class="app-navigation-caption"><?php p($l->t('Personal')); ?></li>
privatemaker marked this conversation as resolved.
Show resolved Hide resolved
<?php foreach ($_['forms']['personal'] as $form):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it weird that the underlying logic to render the setting sections is still executed for admin and personal, even if you visit the personal section and vice versa.

if (isset($form['anchor'])):
$anchor = \OC::$server->getURLGenerator()->linkToRoute('settings.PersonalSettings.index', ['section' => $form['anchor']]);
$class = 'nav-icon-' . $form['anchor'];
$sectionName = $form['section-name']; ?>
Expand All @@ -50,36 +47,8 @@
<?php } ?>
</a>
</li>
<?php
}
}
?>

<?php
if (!empty($_['forms']['admin'])) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Admin right privilege / Administration privileges is broken.

It's possible to make some admin sections visible to groups without granting the users full admin rights. That's called Administration privileges (in Nextcloud) / Admin right privilege (in our Documentation).

  1. Create a group subadmin
  2. Add bob to the subadmin group
  3. Visit Admin -> Administration privileges
  4. Assign a group to a section
  5. Login as bob

Main:

image

Bob see the administration section but only one item.

This PR:

image

Bob does not see the administration section.

It's still possible the admin the section if you know the right url, but there is no link somewhere. The menu item "Administration settings" is only added for users with admin permission, but not administration privileges.

Adding a link below "Settings" is a bit tricky for Administration privileges because you need to figure out the internal url for the given section.

?>
<li class="app-navigation-caption"><?php p($l->t('Administration')); ?></li>
<?php
}
foreach ($_['forms']['admin'] as $form) {
if (isset($form['anchor'])) {
$anchor = \OC::$server->getURLGenerator()->linkToRoute('settings.AdminSettings.index', ['section' => $form['anchor']]);
$class = 'nav-icon-' . $form['anchor'];
$sectionName = $form['section-name']; ?>
<li <?php print_unescaped($form['active'] ? ' class="active"' : ''); ?> data-section-id="<?php print_unescaped($form['anchor']); ?>" data-section-type="admin">
<a href="<?php p($anchor); ?>"<?php print_unescaped($form['active'] ? ' aria-current="page"' : ''); ?>>
<?php if (!empty($form['icon'])) { ?>
<img alt="" src="<?php print_unescaped($form['icon']); ?>">
<span><?php p($form['section-name']); ?></span>
<?php } else { ?>
<span class="no-icon"><?php p($form['section-name']); ?></span>
<?php } ?>
</a>
</li>
<?php
}
}
?>
<?php endif ?>
<?php endforeach ?>
</ul>
</div>
<div id="app-content" tabindex="0" data-active-section-id="<?php print_unescaped($_['activeSectionId']) ?>">
Expand Down
Loading