Admin page split #796

blizzz · 2016-08-09T16:12:55Z

To solve #712

This is not complete, yet, but in a working state with build-in settings and the LDAP one as a 3rd party app using this.

Right now it still does full-page loads, at least there's a route registered to fetch a desired section, needs to be implemented.

Screenshots:

Tasks left:

Catch legacy app that don't register admin settings in info.xml aka make "Additional Settings" work
Transform shipped apps
uh, tests 🔨
don't load app forms if they are disabled aka remove outdated registered settings
highlight current section in the sidebar
~~don't do a full page reload when switching sections~~ (Admin page split #796 (comment))
reconsider DB indexes once every query is in place
~~cream on top (and too much for this PR, yet 10): search for settings~~ (postponed)

Input already welcome @schiessle @jancborchardt and from others of course, too.

* interfaces for the Admin settings (IAdmin) and section (ISection) * SettingsManager service * example setup with LDAP app

mention-bot · 2016-08-09T16:12:56Z

@blizzz, thanks for your PR! By analyzing the annotation information on this pull request, we identified @nickvergessen, @DeepDiver1975 and @LukasReschke to be potential reviewers

schiessle · 2016-08-09T17:56:49Z

Just had a quick look, looks good from a technical point of view. Finally we can get rid of the settings-*.php files and work with proper classes which can also be tested. 🎉

I will try to transfer the monitoring app to a admin settings page tomorrow, that's the best way to test a new API 😃

schiessle · 2016-08-09T17:59:48Z

Catch legacy app that don't register admin settings in info.xml aka make "Additional Settings" work

Is there a fall back for apps we can't adjust in time. We don't have to much time left and we also need to keep 3rdparty apps from the app store in mind.

blizzz · 2016-08-09T20:34:23Z

Catch legacy app that don't register admin settings in info.xml aka make "Additional Settings" work

Is there a fall back for apps we can't adjust in time. We don't have to much time left and we also need to keep 3rdparty apps from the app store in mind.

The plan is to keep this old-style of collecting the forms supported by stashing them into "Additional Settings".

LukasReschke · 2016-08-09T20:38:40Z

db_structure.xml

+			</field>
+
+			<index>
+				<name>admin_sections_id_index</name>


💣 💣 💣 💣 💣 💣 💣 💣 - already used in https://github.com/nextcloud/server/pull/796/files#diff-1d1f96f9afa7544b553d6441f7a3c167R2010, this will fail on many databases.

I guess this should be admin_settings_id_index?

@LukasReschke this should be admin_settings_id_index. Important catch! 🏆

LukasReschke · 2016-08-09T20:44:06Z

reset.php

@@ -0,0 +1,5 @@
+<?php


What's this?

This file should not be there, commited by mistake

LukasReschke · 2016-08-09T20:47:29Z

settings/routes.php

@@ -64,6 +64,8 @@
 		['name' => 'Certificate#removePersonalRootCertificate', 'url' => '/settings/personal/certificate/{certificateIdentifier}', 'verb' => 'DELETE'],
 		['name' => 'Certificate#addSystemRootCertificate', 'url' => '/settings/admin/certificate', 'verb' => 'POST'],
 		['name' => 'Certificate#removeSystemRootCertificate', 'url' => '/settings/admin/certificate/{certificateIdentifier}', 'verb' => 'DELETE'],
+		['name' => 'AdminSettings#index', 'url' => '/settings/admin/{section}', 'verb' => 'GET', 'defaults' => ['section' => 'server']],
+		['name' => 'AdminSettings#form', 'url' => '/settings/admin/{section}', 'verb' => 'GET'],


Used at all?

LukasReschke · 2016-08-09T20:49:32Z

Went over the code quickly already, not a final review since WIP. But looks already pretty awesome 🚀

blizzz · 2016-08-09T21:32:00Z

cc @BernhardPosselt you've shown interest on IRC recently

LukasReschke · 2016-08-10T10:24:15Z

I'll test this by porting the SAML app. PR incoming as well.

schiessle · 2016-08-10T11:33:00Z

lib/public/Settings/IAdmin.php

+	 *
+	 * E.g.: 70
+	 */
+	public function getPriority();


Why do we need getPriority() min ISection and in IAdmin?

One is to sort the Sections in the sidebar, the other to sort the settings forms within the selected section.

good point, forget that we can also group multiple settings from different apps in one section.

Probably just a typo, but we should unify it. Here it says "value between 0 and 100" and for the ISection it says "value between 0 and 99"

schiessle · 2016-08-10T11:35:59Z

I run into a table not found exception when I just switched to this branch. Forget to increase version to trigger db update?

schiessle · 2016-08-10T11:38:09Z

if I go to the sharing settings I get this error in the nextcloud.log:

{"reqId":"Ho2YP2oh2bf1gBTiY1Cx","remoteAddr":"::1","app":"PHP","message":"Undefined index: allowLinks at \/home\/schiesbn\/repos\/nextcloud\/server\/master\/settings\/templates\/admin\/sharing.php#41","level":3,"time":"2016-08-10T11:37:29+00:00","method":"GET","url":"\/master\/index.php\/settings\/admin\/sharing","user":"admin"}
{"reqId":"Ho2YP2oh2bf1gBTiY1Cx","remoteAddr":"::1","app":"PHP","message":"Undefined index: allowLinks at \/home\/schiesbn\/repos\/nextcloud\/server\/master\/settings\/templates\/admin\/sharing.php#45","level":3,"time":"2016-08-10T11:37:29+00:00","method":"GET","url":"\/master\/index.php\/settings\/admin\/sharing","user":"admin"}
{"reqId":"Ho2YP2oh2bf1gBTiY1Cx","remoteAddr":"::1","app":"PHP","message":"Undefined index: allowPublicUpload at \/home\/schiesbn\/repos\/nextcloud\/server\/master\/settings\/templates\/admin\/sharing.php#47","level":3,"time":"2016-08-10T11:37:29+00:00","method":"GET","url":"\/master\/index.php\/settings\/admin\/sharing","user":"admin"}
{"reqId":"Ho2YP2oh2bf1gBTiY1Cx","remoteAddr":"::1","app":"PHP","message":"Undefined index: enforceLinkPassword at \/home\/schiesbn\/repos\/nextcloud\/server\/master\/settings\/templates\/admin\/sharing.php#51","level":3,"time":"2016-08-10T11:37:29+00:00","method":"GET","url":"\/master\/index.php\/settings\/admin\/sharing","user":"admin"}
{"reqId":"Ho2YP2oh2bf1gBTiY1Cx","remoteAddr":"::1","app":"PHP","message":"Undefined index: allowPublicMailNotification at \/home\/schiesbn\/repos\/nextcloud\/server\/master\/settings\/templates\/admin\/sharing.php#59","level":3,"time":"2016-08-10T11:37:29+00:00","method":"GET","url":"\/master\/index.php\/settings\/admin\/sharing","user":"admin"}
{"reqId":"Ho2YP2oh2bf1gBTiY1Cx","remoteAddr":"::1","app":"PHP","message":"Undefined index: allowLinks at \/home\/schiesbn\/repos\/nextcloud\/server\/master\/settings\/templates\/admin\/sharing.php#63","level":3,"time":"2016-08-10T11:37:29+00:00","method":"GET","url":"\/master\/index.php\/settings\/admin\/sharing","user":"admin"}
{"reqId":"Ho2YP2oh2bf1gBTiY1Cx","remoteAddr":"::1","app":"PHP","message":"Undefined index: allowResharing at \/home\/schiesbn\/repos\/nextcloud\/server\/master\/settings\/templates\/admin\/sharing.php#74","level":3,"time":"2016-08-10T11:37:29+00:00","method":"GET","url":"\/master\/index.php\/settings\/admin\/sharing","user":"admin"}
{"reqId":"Ho2YP2oh2bf1gBTiY1Cx","remoteAddr":"::1","app":"PHP","message":"Undefined index: allowGroupSharing at \/home\/schiesbn\/repos\/nextcloud\/server\/master\/settings\/templates\/admin\/sharing.php#79","level":3,"time":"2016-08-10T11:37:29+00:00","method":"GET","url":"\/master\/index.php\/settings\/admin\/sharing","user":"admin"}
{"reqId":"Ho2YP2oh2bf1gBTiY1Cx","remoteAddr":"::1","app":"PHP","message":"Undefined index: onlyShareWithGroupMembers at \/home\/schiesbn\/repos\/nextcloud\/server\/master\/settings\/templates\/admin\/sharing.php#84","level":3,"time":"2016-08-10T11:37:29+00:00","method":"GET","url":"\/master\/index.php\/settings\/admin\/sharing","user":"admin"}
{"reqId":"Ho2YP2oh2bf1gBTiY1Cx","remoteAddr":"::1","app":"PHP","message":"Undefined index: allowMailNotification at \/home\/schiesbn\/repos\/nextcloud\/server\/master\/settings\/templates\/admin\/sharing.php#89","level":3,"time":"2016-08-10T11:37:29+00:00","method":"GET","url":"\/master\/index.php\/settings\/admin\/sharing","user":"admin"}
{"reqId":"Ho2YP2oh2bf1gBTiY1Cx","remoteAddr":"::1","app":"PHP","message":"Undefined index: allowShareDialogUserEnumeration at \/home\/schiesbn\/repos\/nextcloud\/server\/master\/settings\/templates\/admin\/sharing.php#104","level":3,"time":"2016-08-10T11:37:29+00:00","method":"GET","url":"\/master\/index.php\/settings\/admin\/sharing","user":"admin"}
{"reqId":"Ho2YP2oh2bf1gBTiY1Cx","remoteAddr":"::1","app":"PHP","message":"Undefined index: fileSharingSettings at \/home\/schiesbn\/repos\/nextcloud\/server\/master\/settings\/templates\/admin\/sharing.php#108","level":3,"time":"2016-08-10T11:37:29+00:00","method":"GET","url":"\/master\/index.php\/settings\/admin\/sharing","user":"admin"}

schiessle · 2016-08-10T11:45:46Z

If I start with a complete fresh installation my app (ServerInfo) doesn't get added to the admin page. I first have to increase the version in the info.xml to run the update for the app at least once. New apps should be added directly.

schiessle · 2016-08-10T11:50:31Z

If I change the priority, both in the IAdmin and the ISettings implementation and increase the version number in the info.xml the update will add the same app a second time to oc_admin_settings with the new priority. I would expect that it gets replaced.

In oc_admin_section nothing change, I still have only one entry with the previous priority. But maybe this is not needed anyway, see my question here #796 (comment)

schiessle · 2016-08-10T11:51:52Z

Sorry for the many comments. But this was just the stuff I discovered while creating the admin page for the ServerInfo app. Beside that it was super easy and the API feels really good!

blizzz · 2016-08-10T12:25:44Z

Currently working on switching from OCP\Template to TemplateResponse as return value form IAdmin, because it's nicer and already present in several apps. Changed user_ldap locally successful, now trying to adjust updatenotifications, where i somewhat struggle with autoloading yet.

UPDATE: done

@LukasReschke

* bump version to ensure tables are created * make updatenotification app use settings api * change IAdmin::render() to getForm() and change return type from Template to TemplateResponse * adjust User_LDAP accordingly, as well as built-in forms * add IDateTimeFormatter to AppFramework/DependencyInjection/DIContainer.php. This is important so that \OC::$server->query() is able to resolve the constructor parameters. We should ensure that all OCP/* stuff that is available from \OC::$server is available here. Kudos to @LukasReschke * make sure apps that have settings info in their info.xml are loaded before triggering adding the settings setup method

Makes this page a single page as implemented in nextcloud/server#796

blizzz · 2016-08-16T15:17:40Z

This was done for the mail settings but a lot of other settings, including the sharing settings are mission... @blizzz am I right?

No, they are split up. Some parts are in Server, some are in Additional Setting. And Sharing settings are in Sharing.

blizzz · 2016-08-16T15:21:00Z

Just saw that this still happens:

move settings to a seperate page in the admin settings survey_client#13

@schiessle wrong link?

schiessle · 2016-08-16T15:21:07Z

No, they are split up. Some parts are in Server, some are in Additional Setting. And Sharing settings are in Sharing.

yes, I was to fast and edited my comment right before you commented... Still it seems like we removed here setting parameters from the general admin page which we added in no other place. At least I discovered it for sharing and just at the moment for encryption: https://github.com/nextcloud/server/pull/796/files#diff-7ce542aade9c380d3b2e16162d559256L132 So the encryption settings no longer show the encryption modules.

schiessle · 2016-08-16T15:22:04Z

@blizzz why do you still see the old link? 😕 Edited the post directly after I created it... This is the correct link: #796 (comment)

blizzz · 2016-08-16T15:24:26Z

@blizzz why do you still see the old link. Edited the post directly after I created it... This is the correct link: #796 (comment)

Glitch in the matrix?

blizzz · 2016-08-16T15:25:43Z

@schiessle thx for re-checking, will fix it.

blizzz · 2016-08-16T15:59:56Z

@schiessle done

LukasReschke · 2016-08-16T16:15:05Z

Conflicts :)

…ed if available

blizzz · 2016-08-16T16:33:15Z

Conflicts :)

✌️

schiessle · 2016-08-16T18:03:00Z

looks good now 👍

LukasReschke · 2016-08-16T18:27:38Z

LGTM

LukasReschke · 2016-08-16T18:56:10Z

Oh. Backporting this is fun 😨 – So many conflicts…

LukasReschke · 2016-08-16T19:12:28Z

Backport attempt at #894, let's see…

Use new admin page module Makes this page a single page as implemented in nextcloud/server#796 specify namespace for autoloader IAdmin is now ISettings

Admin page split

Initial work on Apps page split:

ceeb44b

* interfaces for the Admin settings (IAdmin) and section (ISection) * SettingsManager service * example setup with LDAP app

blizzz added the 2. developing Work in progress label Aug 9, 2016

blizzz added this to the Nextcloud 11.0 milestone Aug 9, 2016

LukasReschke reviewed Aug 9, 2016
View reviewed changes

fix duplicated db index name

9d9c540

LukasReschke reviewed Aug 9, 2016
View reviewed changes

remove mistakenly added file

97df444

LukasReschke reviewed Aug 9, 2016
View reviewed changes

Fallback for legacy settings. They are placed into Additional Settings

518545f

blizzz mentioned this pull request Aug 9, 2016

Split up Admin page into groups #712

Closed

schiessle reviewed Aug 10, 2016
View reviewed changes

schiessle mentioned this pull request Aug 10, 2016

move server info to the admin page nextcloud/serverinfo#18

Merged

1 task

LukasReschke added a commit to nextcloud/user_saml that referenced this pull request Aug 10, 2016

Use new admin page module

dc1438e

Makes this page a single page as implemented in nextcloud/server#796

LukasReschke added a commit to nextcloud/user_saml that referenced this pull request Aug 10, 2016

Use new admin page module

85364ae

Makes this page a single page as implemented in nextcloud/server#796

LukasReschke mentioned this pull request Aug 10, 2016

Use new admin page module nextcloud/user_saml#29

Merged

fix missing parameters in sharing settings page

58530ab

add missing encryption modules to settings

f76b64d

blizzz added 2 commits August 16, 2016 18:26

for new we cannot have nested settings, default module is only append…

4946cc2

…ed if available

Merge branch 'master' into implement_712

717e222

adjust tests to latest changes

4943441

LukasReschke merged commit 2d861c9 into master Aug 16, 2016

LukasReschke deleted the implement_712 branch August 16, 2016 18:28

LukasReschke mentioned this pull request Aug 16, 2016

[Stable10] Backport 796 #894

Merged

MorrisJobke mentioned this pull request Aug 17, 2016

Files external settings polish #787

Merged

blizzz pushed a commit to nextcloud/user_saml that referenced this pull request Aug 17, 2016

Backport of #29 to stable10

157dd1d

Use new admin page module Makes this page a single page as implemented in nextcloud/server#796 specify namespace for autoloader IAdmin is now ISettings

blizzz mentioned this pull request Aug 17, 2016

Backport of #29 to stable10 nextcloud/user_saml#31

Merged

MariusBluem mentioned this pull request Aug 18, 2016

Make navigating on the admin page feel more responsive (either by faster page loads or by SPA) #953

Closed

juliushaertl mentioned this pull request Aug 19, 2016

Theming does not longer show right Name and Slogan after upgrade to 10 RC1 #949

Closed

GitHubUser4234 pushed a commit to GitHubUser4234/server that referenced this pull request Aug 30, 2016

Merge pull request nextcloud#796 from nextcloud/implement_712

7dfa4ed

Admin page split

MorrisJobke mentioned this pull request May 10, 2017

SSL menu is now shown for files_external #4800

Merged

snyk-bot mentioned this pull request Jun 11, 2022

[Snyk] Upgrade clipboard from 2.0.8 to 2.0.11 JazzEd-EdTech/master#177

Open

joshtrichards mentioned this pull request Nov 16, 2023

[Bug]: Error message when entering a configuration menu #41513

Closed

8 tasks

Admin page split #796

Admin page split #796

Conversation

blizzz commented Aug 9, 2016 • edited Loading

mention-bot commented Aug 9, 2016

schiessle commented Aug 9, 2016 • edited Loading

schiessle commented Aug 9, 2016

blizzz commented Aug 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukasReschke commented Aug 9, 2016

blizzz commented Aug 9, 2016

LukasReschke commented Aug 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schiessle commented Aug 10, 2016

schiessle commented Aug 10, 2016

schiessle commented Aug 10, 2016

schiessle commented Aug 10, 2016

schiessle commented Aug 10, 2016

blizzz commented Aug 10, 2016 • edited Loading

blizzz commented Aug 16, 2016

blizzz commented Aug 16, 2016

schiessle commented Aug 16, 2016

schiessle commented Aug 16, 2016 • edited Loading

blizzz commented Aug 16, 2016

blizzz commented Aug 16, 2016

blizzz commented Aug 16, 2016

LukasReschke commented Aug 16, 2016

blizzz commented Aug 16, 2016

schiessle commented Aug 16, 2016

LukasReschke commented Aug 16, 2016

LukasReschke commented Aug 16, 2016

LukasReschke commented Aug 16, 2016

blizzz commented Aug 9, 2016 •

edited

Loading

schiessle commented Aug 9, 2016 •

edited

Loading

blizzz commented Aug 10, 2016 •

edited

Loading

schiessle commented Aug 16, 2016 •

edited

Loading