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.

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

Move settings registration to info.xml + merge 'personal' & 'admin' settings #26449

Merged
merged 66 commits into from Feb 7, 2017

Conversation

Projects
None yet
8 participants
@tomneedham
Member

tomneedham commented Oct 23, 2016

Description

Remove the legacy OC_App::registerAdmin() and OC_App::registerPersonal() calls in favour of a info.xml based approach. Apps will be able to register 'personal' and 'admin' settings panels by specifying class names in an XML element in the info.xml.

This also helps up break down our existing personal and admin settings page controllers into smaller chunks which can be tested easier.

Also the personal and settings pages have been combined (visually) although the routing remains the same.

Related Issue

#24188

Motivation and Context

See #24188

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

WIP (continuing w/c 24/10/16):

  • Fix registering of IPanels for core, and for Apps
  • Decide on category names
  • Add tests for SettingsManager ⚙
  • Add in catch for legacy calls to OC_App:registerPersonal|Admin()
  • Port over existing settings panels to IPanel and ISection
  • Write documentation updates ✏️

@tomneedham tomneedham self-assigned this Oct 23, 2016

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Oct 23, 2016

@tomneedham, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bartv2, @Kondou-ger and @LukasReschke to be potential reviewers.

@tomneedham, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bartv2, @Kondou-ger and @LukasReschke to be potential reviewers.

Show outdated Hide outdated lib/private/Settings/SettingsManager.php
return [];
}
foreach($panels as $panelClassName) {
// Attempt to load the panel

This comment has been minimized.

@PVince81

PVince81 Oct 24, 2016

Member

did you switch to spaces instead of tabs ? (sorry for being that guy 😄)

@PVince81

PVince81 Oct 24, 2016

Member

did you switch to spaces instead of tabs ? (sorry for being that guy 😄)

This comment has been minimized.

@tomneedham

tomneedham Nov 5, 2016

Member

Maybe..... ;) Sorted.

@tomneedham

tomneedham Nov 5, 2016

Member

Maybe..... ;) Sorted.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Oct 24, 2016

Member

Great stuff. Will this be backward compatible ? Basically if apps still provide settings as before, have code that auto-register those through the new settings manager.

Member

PVince81 commented Oct 24, 2016

Great stuff. Will this be backward compatible ? Basically if apps still provide settings as before, have code that auto-register those through the new settings manager.

@tomneedham

This comment has been minimized.

Show comment
Hide comment
@tomneedham

tomneedham Nov 5, 2016

Member

Will this be backward compatible ?

Yes. We can catch the old calls and group them together.

Member

tomneedham commented Nov 5, 2016

Will this be backward compatible ?

Yes. We can catch the old calls and group them together.

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 Nov 5, 2016

Member
Parse error: ./tests/lib/Settings/ManagerTest.php:43

    41|   public function testLoadPanels() {

    42| 

  > 43|   public function testGetPanels() {}

    44| 

    45|   public function testSortOrder() {}

Unexpected 'public' (T_PUBLIC)

make: *** [test-php-lint] Error 1
Member

DeepDiver1975 commented Nov 5, 2016

Parse error: ./tests/lib/Settings/ManagerTest.php:43

    41|   public function testLoadPanels() {

    42| 

  > 43|   public function testGetPanels() {}

    44| 

    45|   public function testSortOrder() {}

Unexpected 'public' (T_PUBLIC)

make: *** [test-php-lint] Error 1

@DeepDiver1975 DeepDiver1975 added this to the 9.2 milestone Nov 5, 2016

Show outdated Hide outdated lib/private/Settings/SettingsManager.php
],
'admin' => []
'admin' => [
'OC\Settings\Panels\Admin\Legacy',

This comment has been minimized.

@DeepDiver1975

DeepDiver1975 Nov 5, 2016

Member

Please use

use OC\Settings\Panels\Admin\Legacy;

....

Legacy::class
@DeepDiver1975

DeepDiver1975 Nov 5, 2016

Member

Please use

use OC\Settings\Panels\Admin\Legacy;

....

Legacy::class
Show outdated Hide outdated lib/private/Settings/SettingsManager.php
@@ -161,6 +166,8 @@ private function getBuiltInPanels() {
private function getBuiltInPanel($className) {
$panels = [
'OC\Settings\Panels\Personal\Profile' => new \OC\Settings\Panels\Personal\Profile($this->config, $this->groupManager, $this->userSession),

This comment has been minimized.

@DeepDiver1975

DeepDiver1975 Nov 5, 2016

Member

is that necessary? the di magic should be capable of instantiating all classes

@DeepDiver1975

DeepDiver1975 Nov 5, 2016

Member

is that necessary? the di magic should be capable of instantiating all classes

Show outdated Hide outdated settings/Controller/SettingsPageController.php
@@ -59,8 +60,8 @@ public function __construct($appName,
* @param string $sectionID
* @return \OCP\TemplateResponse
*/
public function getPersonal($sectionID='general') {
$this->currentSectionID = $sectionID;
public function getPersonal($sectionid) {

This comment has been minimized.

@DeepDiver1975

DeepDiver1975 Nov 5, 2016

Member

$sectionId

@DeepDiver1975

DeepDiver1975 Nov 5, 2016

Member

$sectionId

Show outdated Hide outdated settings/Panels/Admin/Legacy.php
class Legacy implements IPanel {
public function __construct() {}

This comment has been minimized.

@DeepDiver1975

DeepDiver1975 Nov 5, 2016

Member

empty? kill it

@DeepDiver1975

DeepDiver1975 Nov 5, 2016

Member

empty? kill it

Show outdated Hide outdated lib/private/Settings/Section.php
*/
class Section implements ISection {
protected $id, $name, $priority;

This comment has been minimized.

@DeepDiver1975

DeepDiver1975 Nov 5, 2016

Member

each on it's own line please and also add

/** @var int */
@DeepDiver1975

DeepDiver1975 Nov 5, 2016

Member

each on it's own line please and also add

/** @var int */
Show outdated Hide outdated lib/public/Settings/ISettingsManager.php
* @since 9.2
* @return array of OCP\Settings\ISection (sorted by priority)
*/
public function getPersonalSections();

This comment has been minimized.

@DeepDiver1975

DeepDiver1975 Nov 5, 2016

Member

indent looks off

@DeepDiver1975

DeepDiver1975 Nov 5, 2016

Member

indent looks off

/**
* Controllers
*/
$container->registerService('SettingsPageController', function(IContainer $c) {
return new SettingsPageController(

This comment has been minimized.

@DeepDiver1975

DeepDiver1975 Nov 5, 2016

Member

controllers should be loadable via di magic - no need to register them anymore

@DeepDiver1975

DeepDiver1975 Nov 5, 2016

Member

controllers should be loadable via di magic - no need to register them anymore

Show outdated Hide outdated settings/Controller/SettingsPageController.php
*/
public function __construct($appName,
IRequest $request,
ISettingsManager $settingsManager,

This comment has been minimized.

@DeepDiver1975

DeepDiver1975 Nov 5, 2016

Member

indent

Show outdated Hide outdated settings/Controller/SettingsPageController.php
} else if($type == 'admin') {
$sections = $this->settingsManager->getAdminSections();
$panels = $this->settingsManager->getAdminPanels($this->currentSectionID);
}

This comment has been minimized.

@DeepDiver1975

DeepDiver1975 Nov 5, 2016

Member

missing else block

@DeepDiver1975

DeepDiver1975 Nov 5, 2016

Member

missing else block

@tomneedham

This comment has been minimized.

Show comment
Hide comment
@tomneedham

tomneedham Nov 8, 2016

Member

@felixheidecke Hey! Maybe you can help me decide how we should categorise the existing settings panels in a way that makes most sense for the user?

See the random list I've been working on here: https://github.com/owncloud/core/pull/26449/files#diff-879bc94ed8e636501bd669b9f1d9f17cR147

Basically, legacy uses of registerPersonal and registerAdmin will just go into the 'additional' sections until they get moved to relevant ones.

Member

tomneedham commented Nov 8, 2016

@felixheidecke Hey! Maybe you can help me decide how we should categorise the existing settings panels in a way that makes most sense for the user?

See the random list I've been working on here: https://github.com/owncloud/core/pull/26449/files#diff-879bc94ed8e636501bd669b9f1d9f17cR147

Basically, legacy uses of registerPersonal and registerAdmin will just go into the 'additional' sections until they get moved to relevant ones.

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 Nov 9, 2016

Member

Please fix ci - thx

Member

DeepDiver1975 commented Nov 9, 2016

Please fix ci - thx

Show outdated Hide outdated settings/Panels/Admin/BackgroundJobs.php
}
public function getName() {
return 'Backgroudn Jobs';

This comment has been minimized.

@DeepDiver1975

DeepDiver1975 Nov 9, 2016

Member

typo - do we need translations on this?

@DeepDiver1975

DeepDiver1975 Nov 9, 2016

Member

typo - do we need translations on this?

Show outdated Hide outdated settings/templates/panels/personal/profile.php
@@ -1,3 +1,8 @@
<?php
\OC_Util::addScript('files', 'jquery.fileupload');

This comment has been minimized.

@DeepDiver1975

DeepDiver1975 Nov 9, 2016

Member
script('files', 'jquery.fileupload')
@DeepDiver1975

DeepDiver1975 Nov 9, 2016

Member
script('files', 'jquery.fileupload')
@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 Nov 9, 2016

Member

on personal page:

personal.js?v=f23f455…:367 Uncaught TypeError: Cannot read property 'AuthTokenCollection' of undefined
Member

DeepDiver1975 commented Nov 9, 2016

on personal page:

personal.js?v=f23f455…:367 Uncaught TypeError: Cannot read property 'AuthTokenCollection' of undefined
@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 Nov 9, 2016

Member

and

personal.js?v=f23f455…:288 Uncaught TypeError: $(...).fileupload is not a function
Member

DeepDiver1975 commented Nov 9, 2016

and

personal.js?v=f23f455…:288 Uncaught TypeError: $(...).fileupload is not a function
@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 Nov 9, 2016

Member

on admin all section headings are duplicated

Member

DeepDiver1975 commented Nov 9, 2016

on admin all section headings are duplicated

@tomneedham

This comment has been minimized.

Show comment
Hide comment
@tomneedham

tomneedham Nov 9, 2016

Member

Not too hot on the authtoken stuff but I think we need a fix for this: https://github.com/owncloud/core/pull/26449/files#diff-a1c52fafc7d1f682c8849c5c62297682L376

We could add a check for this in the controller, find the relevant section given the panel id and then redirect to it?

Member

tomneedham commented Nov 9, 2016

Not too hot on the authtoken stuff but I think we need a fix for this: https://github.com/owncloud/core/pull/26449/files#diff-a1c52fafc7d1f682c8849c5c62297682L376

We could add a check for this in the controller, find the relevant section given the panel id and then redirect to it?

@tomneedham

This comment has been minimized.

Show comment
Hide comment
@tomneedham

tomneedham Nov 9, 2016

Member

Also https://github.com/owncloud/core/blob/master/settings/routes.php#L41 is being overwritten by the route below which means the authtoken panel is broken at the moment.

Member

tomneedham commented Nov 9, 2016

Also https://github.com/owncloud/core/blob/master/settings/routes.php#L41 is being overwritten by the route below which means the authtoken panel is broken at the moment.

@tomneedham

This comment has been minimized.

Show comment
Hide comment
@tomneedham

tomneedham Nov 14, 2016

Member
  • Fix file sharing settings saving / initial value retrieval
Member

tomneedham commented Nov 14, 2016

  • Fix file sharing settings saving / initial value retrieval
@felixheidecke

This comment has been minimized.

Show comment
Hide comment
@felixheidecke

felixheidecke Nov 17, 2016

Contributor

Unifying personal and administrator under a general settings section is a good idea, since its a memorized term in modern UIs. (See mobile phones, desktop software and such).

Moving to separate sites is quiet fine, since you usualy don't casualy browse the setting but rather look for specific settings.

The only downside is the page reloads but this can be resolved with ajax in the future.

@tomneedham If you need more design / UI input, pls ask.

Contributor

felixheidecke commented Nov 17, 2016

Unifying personal and administrator under a general settings section is a good idea, since its a memorized term in modern UIs. (See mobile phones, desktop software and such).

Moving to separate sites is quiet fine, since you usualy don't casualy browse the setting but rather look for specific settings.

The only downside is the page reloads but this can be resolved with ajax in the future.

@tomneedham If you need more design / UI input, pls ask.

@tomneedham

This comment has been minimized.

Show comment
Hide comment
@tomneedham

tomneedham Nov 19, 2016

Member

Unifying personal and administrator under a general settings section is a good idea, since its a memorized term in modern UIs. (See mobile phones, desktop software and such).

Awesome thanks @felixheidecke. We'll get this one in first which has backend changes then we can work towards that + ajax loading :)

Member

tomneedham commented Nov 19, 2016

Unifying personal and administrator under a general settings section is a good idea, since its a memorized term in modern UIs. (See mobile phones, desktop software and such).

Awesome thanks @felixheidecke. We'll get this one in first which has backend changes then we can work towards that + ajax loading :)

@tomneedham

This comment has been minimized.

Show comment
Hide comment
@tomneedham

tomneedham Nov 19, 2016

Member

Still to be resolved is the issues of existing settings panels using routes of the format GET /settings/personal|admin/$something as this collides with the new route to fetch the settings page.

Member

tomneedham commented Nov 19, 2016

Still to be resolved is the issues of existing settings panels using routes of the format GET /settings/personal|admin/$something as this collides with the new route to fetch the settings page.

Show outdated Hide outdated apps/files_external/appinfo/info.xml
@@ -39,5 +39,6 @@
</commands>
<settings>
<admin>OCA\Files_External\Panels\Admin</admin>
<admin>OCA\Files_External\Panels\Personal</admin>

This comment has been minimized.

@DeepDiver1975

DeepDiver1975 Nov 19, 2016

Member

admin? should be personal right?

@DeepDiver1975

DeepDiver1975 Nov 19, 2016

Member

admin? should be personal right?

Show outdated Hide outdated settings/Panels/Admin/Encryption.php
@@ -32,6 +32,23 @@ public function getPriority() {
public function getPanel() {
$tmpl = new Template('settings', 'panels/admin/encryption');
$tmpl->assign('encryptionEnabled', \OC::$server->getEncryptionManager()->isEnabled());

This comment has been minimized.

@DeepDiver1975

DeepDiver1975 Nov 19, 2016

Member

indent

Show outdated Hide outdated settings/routes.php
['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' => 'LogSettings#setLogLevel', 'url' => '/settings/admin/ajax/log/level', 'verb' => 'POST'],

This comment has been minimized.

@DeepDiver1975

DeepDiver1975 Nov 19, 2016

Member

what about removing admin/ajax?

so in this case: /settings/log/level

@DeepDiver1975

DeepDiver1975 Nov 19, 2016

Member

what about removing admin/ajax?

so in this case: /settings/log/level

This comment has been minimized.

@tomneedham

tomneedham Nov 19, 2016

Member

I chose not to because I was worried about breaking routes like this https://github.com/owncloud/core/blob/master/settings/js/certificates.js#L5 which take in the settings type, but if this one is safe I could move it.

@tomneedham

tomneedham Nov 19, 2016

Member

I chose not to because I was worried about breaking routes like this https://github.com/owncloud/core/blob/master/settings/js/certificates.js#L5 which take in the settings type, but if this one is safe I could move it.

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 Nov 21, 2016

Member

nice progress @tomneedham

Next steps:

Member

DeepDiver1975 commented Nov 21, 2016

nice progress @tomneedham

Next steps:

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Nov 25, 2016

Member

I assume that will only have one level of sections.
Try and make the number of sections in the left sidebar small.
Within the actual page we can still display multiple sub-sections.

@felixheidecke can you check the sections we have a provide a list of revamped sections ?

@DeepDiver1975 @tomneedham I'd say let's merge the "admin" and "personal" sections now in this PR. It will reduce testing time as we won't need to test twice (once in this PR and once sections merged again)

Member

PVince81 commented Nov 25, 2016

I assume that will only have one level of sections.
Try and make the number of sections in the left sidebar small.
Within the actual page we can still display multiple sub-sections.

@felixheidecke can you check the sections we have a provide a list of revamped sections ?

@DeepDiver1975 @tomneedham I'd say let's merge the "admin" and "personal" sections now in this PR. It will reduce testing time as we won't need to test twice (once in this PR and once sections merged again)

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 Nov 25, 2016

Member

@DeepDiver1975 @tomneedham I'd say let's merge the "admin" and "personal" sections now in this PR. It will reduce testing time as we won't need to test twice (once in this PR and once sections merged again)

Agreed!

Member

DeepDiver1975 commented Nov 25, 2016

@DeepDiver1975 @tomneedham I'd say let's merge the "admin" and "personal" sections now in this PR. It will reduce testing time as we won't need to test twice (once in this PR and once sections merged again)

Agreed!

@tomneedham

This comment has been minimized.

Show comment
Hide comment
Member

tomneedham commented Dec 1, 2016

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Dec 1, 2016

Member

Hmm, interesting. I didn't think of having "Admin" and "Personal" as main sections, I thought we'd just have all sections directly within the sidebar without differenciation. But maybe your approach is better to keep some structure. So your proposal looks good.

Member

PVince81 commented Dec 1, 2016

Hmm, interesting. I didn't think of having "Admin" and "Personal" as main sections, I thought we'd just have all sections directly within the sidebar without differenciation. But maybe your approach is better to keep some structure. So your proposal looks good.

@tomneedham

This comment has been minimized.

Show comment
Hide comment
@tomneedham

tomneedham Dec 1, 2016

Member

Hmm, interesting. I didn't think of having "Admin" and "Personal" as main sections, I thought we'd just have all sections directly within the sidebar without differenciation

Ah! Funny how two different minds think. This was the easiest option from an implementation point of view, but also keeps it clear for the user.

Member

tomneedham commented Dec 1, 2016

Hmm, interesting. I didn't think of having "Admin" and "Personal" as main sections, I thought we'd just have all sections directly within the sidebar without differenciation

Ah! Funny how two different minds think. This was the easiest option from an implementation point of view, but also keeps it clear for the user.

@tomneedham

This comment has been minimized.

Show comment
Hide comment
Member

tomneedham commented Dec 1, 2016

screen shot 2016-12-01 at 17 50 41

Show outdated Hide outdated .user.ini
@@ -1,5 +1,5 @@
upload_max_filesize=513M
post_max_size=513M
upload_max_filesize=700M

This comment has been minimized.

@DeepDiver1975

DeepDiver1975 Dec 2, 2016

Member

please revert this

@DeepDiver1975

DeepDiver1975 Dec 2, 2016

Member

please revert this

Show outdated Hide outdated apps/encryption/lib/Panels/Admin.php
}
}
?>

This comment has been minimized.

@DeepDiver1975

DeepDiver1975 Dec 2, 2016

Member

no closing php tag please

@DeepDiver1975

DeepDiver1975 Dec 2, 2016

Member

no closing php tag please

Show outdated Hide outdated apps/encryption/lib/Panels/Personal.php
}
}
?>

This comment has been minimized.

Show outdated Hide outdated apps/federatedfilesharing/appinfo/info.xml
@@ -11,4 +11,8 @@
<dependencies>
<owncloud min-version="9.2" max-version="9.2" />
</dependencies>
<settings>
<admin>OCA\FederatedFileSharing\AdminPanel</admin>
<personal>OCA\FederatedFileSharing\PersonalPanel</personal>

This comment has been minimized.

@DeepDiver1975

DeepDiver1975 Dec 2, 2016

Member

indent

@tomneedham

This comment has been minimized.

Show comment
Hide comment
@tomneedham

tomneedham Dec 2, 2016

Member

@DeepDiver1975 I'm slightly concerned about my port of the encryption app and how it handles displaying the appropriate settings panels. Can we get someone who is up to speed on that to give this a good test?

Member

tomneedham commented Dec 2, 2016

@DeepDiver1975 I'm slightly concerned about my port of the encryption app and how it handles displaying the appropriate settings panels. Can we get someone who is up to speed on that to give this a good test?

@tomneedham tomneedham changed the title from [WIP] Move settings registration to info.xml to Move settings registration to info.xml Dec 2, 2016

@tomneedham tomneedham changed the title from Move settings registration to info.xml to Move settings registration to info.xml + merge 'personal' & 'admin' settings Dec 2, 2016

@tomneedham tomneedham referenced this pull request Dec 2, 2016

Closed

Add icons to settings sections in admin/personal #26759

4 of 6 tasks complete
@tomneedham

This comment has been minimized.

Show comment
Hide comment
@tomneedham

tomneedham Dec 2, 2016

Member
  • BUG: Settings not accessible for non admin users: get 403 access denied
Member

tomneedham commented Dec 2, 2016

  • BUG: Settings not accessible for non admin users: get 403 access denied
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Dec 5, 2016

Member

Unrelated failure... have seen this on many PRs

14:38:32 name: smb-windows
14:39:33 ....................
14:39:33 [ERROR] Server not reachable
14:39:33 stop: stop-smb-windows.sh
Member

PVince81 commented Dec 5, 2016

Unrelated failure... have seen this on many PRs

14:38:32 name: smb-windows
14:39:33 ....................
14:39:33 [ERROR] Server not reachable
14:39:33 stop: stop-smb-windows.sh

@davitol davitol self-requested a review Feb 3, 2017

@davitol

davitol requested changes Feb 3, 2017 edited

  • Users cannot change their Passwords

  • 'En' instead of 'English' in dropdown menu (same in other languages)

screen shot 2017-02-03 at 13 22 58

  • 'Test email settings' button 'fakes' result. Green when no e-mail was set

screen shot 2017-02-03 at 13 41 37

  • Left side Personal Section is useless
    screen shot 2017-02-03 at 13 47 27

and create these logs when clicking on it:

{"reqId":"h8TWP1aBEEEH57NFFHrk","remoteAddr":"82.159.139.58","app":"PHP","message":"Illegal offset type in isset or empty at \/opt\/owncloud\/lib\/private\/Cache\/CappedMemoryCache.php#41","level":3,"time":"2017-02-03T13:10:16+00:00","method":"GET","url":"\/index.php\/settings\/personal?sectionid=general","user":"admin"}
{"reqId":"h8TWP1aBEEEH57NFFHrk","remoteAddr":"82.159.139.58","app":"PHP","message":"Object of class OC\\User\\User could not be converted to string at \/opt\/owncloud\/lib\/composer\/doctrine\/dbal\/lib\/Doctrine\/DBAL\/Driver\/PDOStatement.php#91","level":3,"time":"2017-02-03T13:10:16+00:00","method":"GET","url":"\/index.php\/settings\/personal?sectionid=general","user":"admin"}
{"reqId":"h8TWP1aBEEEH57NFFHrk","remoteAddr":"82.159.139.58","app":"PHP","message":"Illegal offset type at \/opt\/owncloud\/lib\/private\/Cache\/CappedMemoryCache.php#49","level":3,"time":"2017-02-03T13:10:16+00:00","method":"GET","url":"\/index.php\/settings\/personal?sectionid=general","user":"admin"}
  • SideBar for Admin menu; Only general section works

screen shot 2017-02-03 at 14 17 58

screen shot 2017-02-03 at 14 47 15

@tomneedham

This comment has been minimized.

Show comment
Hide comment
@tomneedham

tomneedham Feb 4, 2017

Member

Thanks @davitol for checking this. In one of my later commits I changed a variable name and it killed the routing for anything other than 'general' settings. Fixed that now, and the route for the setting the password.

Member

tomneedham commented Feb 4, 2017

Thanks @davitol for checking this. In one of my later commits I changed a variable name and it killed the routing for anything other than 'general' settings. Fixed that now, and the route for the setting the password.

@tomneedham

This comment has been minimized.

Show comment
Hide comment
@tomneedham

tomneedham Feb 4, 2017

Member

Can you please verify the email functionality against master? The route is being fired correctly and this PR doesn't touch the logic here so bit bewildered as to why it would change, but if its not a problem on master I'll dig into it.

Member

tomneedham commented Feb 4, 2017

Can you please verify the email functionality against master? The route is being fired correctly and this PR doesn't touch the logic here so bit bewildered as to why it would change, but if its not a problem on master I'll dig into it.

@davitol

This comment has been minimized.

Show comment
Hide comment
@davitol

davitol Feb 7, 2017

Contributor

@tomneedham

  • Clicking on setting menu, it spits the following logs in owncloud.log
{"reqId":"ofquu\/bHI\/mXaOSjJly+","remoteAddr":"82.159.139.58","app":"PHP","message":"Illegal offset type in isset or empty at \/opt\/owncloud\/lib\/private\/Cache\/CappedMemoryCache.php#41","level":3,"time":"2017-02-07T09:13:27+00:00","method":"GET","url":"\/index.php\/settings\/personal","user":"admin"}
{"reqId":"ofquu\/bHI\/mXaOSjJly+","remoteAddr":"82.159.139.58","app":"PHP","message":"Object of class OC\\User\\User could not be converted to string at \/opt\/owncloud\/lib\/composer\/doctrine\/dbal\/lib\/Doctrine\/DBAL\/Driver\/PDOStatement.php#91","level":3,"time":"2017-02-07T09:13:27+00:00","method":"GET","url":"\/index.php\/settings\/personal","user":"admin"}
{"reqId":"ofquu\/bHI\/mXaOSjJly+","remoteAddr":"82.159.139.58","app":"PHP","message":"Illegal offset type at \/opt\/owncloud\/lib\/private\/Cache\/CappedMemoryCache.php#49","level":3,"time":"2017-02-07T09:13:27+00:00","method":"GET","url":"\/index.php\/settings\/personal","user":"admin"}
  • 'En' instead of 'English' in dropdown menu (same in other languages)

  • Clicking on additional menu:

screen shot 2017-02-07 at 10 19 17

  • IMHO if apps are going to be managed from the settings menu, the apps+ button should be removed from the left side

screen shot 2017-02-07 at 10 22 52

  • The following error keeps showing when clicking on any admin section

screen shot 2017-02-03 at 14 47 15

Contributor

davitol commented Feb 7, 2017

@tomneedham

  • Clicking on setting menu, it spits the following logs in owncloud.log
{"reqId":"ofquu\/bHI\/mXaOSjJly+","remoteAddr":"82.159.139.58","app":"PHP","message":"Illegal offset type in isset or empty at \/opt\/owncloud\/lib\/private\/Cache\/CappedMemoryCache.php#41","level":3,"time":"2017-02-07T09:13:27+00:00","method":"GET","url":"\/index.php\/settings\/personal","user":"admin"}
{"reqId":"ofquu\/bHI\/mXaOSjJly+","remoteAddr":"82.159.139.58","app":"PHP","message":"Object of class OC\\User\\User could not be converted to string at \/opt\/owncloud\/lib\/composer\/doctrine\/dbal\/lib\/Doctrine\/DBAL\/Driver\/PDOStatement.php#91","level":3,"time":"2017-02-07T09:13:27+00:00","method":"GET","url":"\/index.php\/settings\/personal","user":"admin"}
{"reqId":"ofquu\/bHI\/mXaOSjJly+","remoteAddr":"82.159.139.58","app":"PHP","message":"Illegal offset type at \/opt\/owncloud\/lib\/private\/Cache\/CappedMemoryCache.php#49","level":3,"time":"2017-02-07T09:13:27+00:00","method":"GET","url":"\/index.php\/settings\/personal","user":"admin"}
  • 'En' instead of 'English' in dropdown menu (same in other languages)

  • Clicking on additional menu:

screen shot 2017-02-07 at 10 19 17

  • IMHO if apps are going to be managed from the settings menu, the apps+ button should be removed from the left side

screen shot 2017-02-07 at 10 22 52

  • The following error keeps showing when clicking on any admin section

screen shot 2017-02-03 at 14 47 15

@davitol

This comment has been minimized.

Show comment
Hide comment
@davitol

davitol Feb 7, 2017

Contributor

@tomneedham

Thanks @davitol for checking this. In one of my later commits I changed a variable name and it killed the routing for anything other than 'general' settings. Fixed that now, and the route for the setting the password.

👍 It is fixed

Can you please verify the email functionality against master? The route is being fired correctly and this PR doesn't touch the logic here so bit bewildered as to why it would change, but if its not a problem on master I'll dig into it.

Now it seems to work fine, cannot reproduce it again, so maybe is a corner case not related with this PR

Contributor

davitol commented Feb 7, 2017

@tomneedham

Thanks @davitol for checking this. In one of my later commits I changed a variable name and it killed the routing for anything other than 'general' settings. Fixed that now, and the route for the setting the password.

👍 It is fixed

Can you please verify the email functionality against master? The route is being fired correctly and this PR doesn't touch the logic here so bit bewildered as to why it would change, but if its not a problem on master I'll dig into it.

Now it seems to work fine, cannot reproduce it again, so maybe is a corner case not related with this PR

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 Feb 7, 2017

Member

IMHO if apps are going to be managed from the settings menu, the apps+ button should be removed from the left side

this is part of another pr: #26763

Member

DeepDiver1975 commented Feb 7, 2017

IMHO if apps are going to be managed from the settings menu, the apps+ button should be removed from the left side

this is part of another pr: #26763

@DeepDiver1975 DeepDiver1975 merged commit cc7944d into master Feb 7, 2017

4 checks passed

Scrutinizer 62 new issues, 180 updated code elements
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@davitol

This comment has been minimized.

Show comment
Hide comment
@davitol

davitol Feb 7, 2017

Contributor

Logs are not shown via webUI:

This PR:

screen shot 2017-02-07 at 13 23 36

Previous versions:
screen shot 2017-02-07 at 13 22 49

Note: TypeError: 'OC.Log is undefined' is spotted in Firebug when we change 'what to log'

Contributor

davitol commented Feb 7, 2017

Logs are not shown via webUI:

This PR:

screen shot 2017-02-07 at 13 23 36

Previous versions:
screen shot 2017-02-07 at 13 22 49

Note: TypeError: 'OC.Log is undefined' is spotted in Firebug when we change 'what to log'

@tomneedham tomneedham deleted the settings-panels branch Feb 23, 2017

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 7, 2017

Member

Raised owncloud/documentation#2922 to update the doc to tell how to use the new registration mechanism for settings panels

Member

PVince81 commented Mar 7, 2017

Raised owncloud/documentation#2922 to update the doc to tell how to use the new registration mechanism for settings panels

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment