introduce names for user backends - via IUserBackend #12819

MorrisJobke · 2014-12-12T16:26:21Z

cc @DeepDiver1975 @icewind1991 @PVince81 @DeepDiver1975

Is this how we should add stuff to a interface that is implemented by third party apps?

MorrisJobke · 2014-12-12T16:27:22Z

PVince81 · 2014-12-12T16:42:37Z

lib/public/iuserinterfacev2.php

+// This means that they should be used by apps instead of the internal ownCloud classes
+namespace OCP;
+
+interface IUserInterfaceV2 extends UserInterface {


Okay... UserInterface... I thought it was about the web UI but actually managing users.
Should probably be called IUserManagement or something else instead. Especially that the "I" is already there for "Interface"

-.- damn I was just blind for this

Maybe IUserBackend? Because we have IUser(represents one user), IUserManger (manages user backends), UserInterface (old version of this but it describes a backend)

MorrisJobke · 2014-12-12T17:08:26Z

@PVince81 I renamed the class ;)

PVince81 · 2014-12-15T09:41:28Z

Will the JS UI call t() on the name to get it translated at display time ?

PVince81 · 2014-12-15T09:42:15Z

👍

I get you cannot / don't want to rename the "UserInterface" interface as it was there already, which is fine.

MorrisJobke · 2014-12-15T13:31:46Z

Will the JS UI call t() on the name to get it translated at display time ?

Not yet. Should it be done in the JS part or in the PHP part also?

PVince81 · 2014-12-15T13:35:02Z

I suggest to do it in the JS part when rendering the results. This way your REST results are closer to the original data and the JS is taking care of presentation.

LukasReschke · 2014-12-15T15:10:44Z

Not sure if that is not something that belongs into implementsAction and the user manager? - Though I have no idea what our goal here is ;-)

MorrisJobke · 2014-12-16T08:23:10Z

Not sure if that is not something that belongs into implementsAction and the user manager? - Though I have no idea what our goal here is ;-)

@DeepDiver1975 @icewind1991 @PVince81 Opinions?

nickvergessen · 2014-12-16T08:39:22Z

lib/public/userinterface.php

+ * Interface UserInterface
+ *
+ * @package OCP
+ * @deprecated Use \OCP\IUserBackend instead


Has this been released already, or is it new in oC8, because then I'd just delete it right away

This one is a bit older ;) It's already in stable5 ;) https://github.com/owncloud/core/blob/stable5/lib/public/userinterface.php

nickvergessen · 2014-12-16T08:43:30Z

Not that strings are not going to be translated, because there is no $l->t() which would add it to transifex

MorrisJobke · 2014-12-16T11:22:53Z

Not that strings are not going to be translated, because there is no $l->t() which would add it to transifex

? There are no translated strings in here. If so, then they are in a new PR that follows this one.

nickvergessen · 2014-12-16T11:26:09Z

I just mean, adding something like t($userBackendName) in JS will not working, unless t('Database'), etc is added somewhere

MorrisJobke · 2014-12-16T12:07:21Z

@nickvergessen That's another problem, that shouldn't be addressed by this PR

DeepDiver1975 · 2014-12-18T08:51:53Z

Not sure if that is not something that belongs into implementsAction and the user manager? - Though I have no idea what our goal here is ;-)

@DeepDiver1975 @icewind1991 @PVince81 Opinions?

I'd follow the approach to introduce an interface just like IApacheBackend - basically interface without extends

MorrisJobke · 2014-12-18T09:55:25Z

@DeepDiver1975 Implemented and tested your approach. Ready for review

blizzz · 2014-12-18T10:12:19Z

apps/user_ldap/user_ldap.php

@@ -27,7 +27,7 @@

 use OCA\user_ldap\lib\BackendUtility;

-class USER_LDAP extends BackendUtility implements \OCP\UserInterface {
+class USER_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserInterface {


this is also necessary for user_proxy.php, i can take care of it

done with fed45af

blizzz · 2014-12-18T10:15:10Z

This wants to be adjusted as well: https://github.com/owncloud/core/blob/master/lib/private/user/manager.php#L282

MorrisJobke · 2014-12-18T12:08:41Z

This wants to be adjusted as well: https://github.com/owncloud/core/blob/master/lib/private/user/manager.php#L282

Why? The additional name requires the new Interface. For other backends you need to add a fallback there. Why change this internal thing if it works already?

blizzz · 2014-12-18T12:12:40Z

This wants to be adjusted as well: https://github.com/owncloud/core/blob/master/lib/private/user/manager.php#L282

Why? The additional name requires the new Interface. For other backends you need to add a fallback there. Why change this internal thing if it works already?

Well, in order to use the backend name, if available, instead of the class name. I.e. the backend would be needed to test if it is an instance of IUserBackend, too. Just as in like the method you used in User.

MorrisJobke · 2014-12-18T12:31:33Z

@blizzz Fixed ;)

blizzz · 2014-12-18T13:01:58Z

lib/private/user/manager.php

@@ -279,10 +279,15 @@ public function countUsers() {
 			if ($backend->implementsActions(\OC_User_Backend::COUNT_USERS)) {
 				$backendusers = $backend->countUsers();
 				if($backendusers !== false) {
-					if(isset($userCountStatistics[get_class($backend)])) {
-						$userCountStatistics[get_class($backend)] += $backendusers;
+					if($this->backend instanceof \OCP\IUserBackend) {


almost. $backend not $this-backend. we're looping through $this->backends.

MorrisJobke · 2014-12-18T14:01:43Z

@blizzz done

MorrisJobke · 2014-12-19T01:09:58Z

Jenkins PR: #12953

MorrisJobke · 2014-12-19T08:56:42Z

@owncloud-bot retest this please

MorrisJobke · 2014-12-19T08:59:10Z

I will have a look

00:14:14.353 
00:14:14.353 There were 2 failures:
00:14:14.353 
00:14:14.353 1) Test\User\Manager::testCountUsersOneBackend
00:14:14.353 Failed asserting that false is true.
00:14:14.353 
00:14:14.353 /var/jenkins/workspace/pull-request-analyser-ng-simple@3/label/SLAVE/tests/lib/user/manager.php:388
00:14:14.354 
00:14:14.354 2) Test\User\Manager::testCountUsersTwoBackends
00:14:14.354 Failed asserting that false is true.
00:14:14.354 
00:14:14.354 /var/jenkins/workspace/pull-request-analyser-ng-simple@3/label/SLAVE/tests/lib/user/manager.php:426

* LDAP with multiple servers also proved backendName

MorrisJobke · 2014-12-19T09:17:50Z

Issue is fixed now :) Ready for review again.

MorrisJobke · 2014-12-19T09:42:32Z

jenkins PR: #12956

scrutinizer-notifier · 2014-12-19T09:54:44Z

The inspection completed: 2 new issues, 6 updated code elements

ghost · 2014-12-19T09:56:08Z

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/5128/

Build result: FAILURE

[...truncated 17 lines...]using GIT_SSH to set credentials using .gitcredentials to set credentials > git config --local credential.helper store --file=/tmp/git1007877746804277364.credentials # timeout=10 > git fetch --tags --progress https://github.com/owncloud/core.git +refs/pull/:refs/remotes/origin/pr/ > git config --local --remove-section credential # timeout=10 > git rev-parse origin/pr/12819/merge^{commit} # timeout=10 > git branch -a --contains 02c9d42ee8e0e8b23fbaaf9fac29d10ac8c01aff # timeout=10 > git rev-parse remotes/origin/pr/12819/merge^{commit} # timeout=10Checking out Revision 02c9d42ee8e0e8b23fbaaf9fac29d10ac8c01aff (origin/pr/12819/merge) > git config core.sparsecheckout # timeout=10 > git checkout -f 02c9d42ee8e0e8b23fbaaf9fac29d10ac8c01aff > git rev-list c508f25f60e5db5bcc8fe0bd962a46b0376c05c2 # timeout=10First time build. Skipping changelog. > git remote # timeout=10 > git submodule init # timeout=10 > git submodule sync # timeout=10 > git config --get remote.origin.url # timeout=10 > git submodule update --init --recursiveTriggering pull-request-analyser-ng-simple » SLAVEpull-request-analyser-ng-simple » SLAVE completed with result FAILUREStarted calculate disk usage of buildFinished Calculation of disk usage of build in 0 secondsStarted calculate disk usage of workspaceFinished Calculation of disk usage of workspace in 1 minute 25 second
Test FAILed.

blizzz · 2014-12-19T10:41:54Z

👍 as soons as Jenkins is OK, too.

MorrisJobke added the 3 - To Review label Dec 12, 2014

MorrisJobke added this to the 8.0-current milestone Dec 12, 2014

LukasReschke mentioned this pull request Dec 12, 2014

User Management - ownCloud 8.0 #12620

Closed

23 tasks

PVince81 reviewed Dec 12, 2014
View reviewed changes

MorrisJobke force-pushed the user-backend-names branch from 26aa3b7 to 0507f11 Compare December 12, 2014 17:08

MorrisJobke force-pushed the user-backend-names branch from 0507f11 to 1d3b9bc Compare December 13, 2014 07:36

MorrisJobke changed the title ~~introduce names for user backends - via IUserInterfacev2~~ introduce names for user backends - via IUserBackend Dec 13, 2014

nickvergessen reviewed Dec 16, 2014
View reviewed changes

MorrisJobke force-pushed the user-backend-names branch from 1d3b9bc to 58b4852 Compare December 18, 2014 09:55

blizzz reviewed Dec 18, 2014
View reviewed changes

MorrisJobke force-pushed the user-backend-names branch from fed45af to c021895 Compare December 18, 2014 12:31

blizzz reviewed Dec 18, 2014
View reviewed changes

MorrisJobke force-pushed the user-backend-names branch from c021895 to 403fcfe Compare December 18, 2014 14:01

MorrisJobke mentioned this pull request Dec 19, 2014

[jenkins only] introduce names for user backends - IUserBackend #12953

Closed

introduce names for user backends - IUserBackend

6da33e1

* LDAP with multiple servers also proved backendName

MorrisJobke force-pushed the user-backend-names branch from 403fcfe to 6da33e1 Compare December 19, 2014 09:17

MorrisJobke mentioned this pull request Dec 19, 2014

[jenkins only] introduce names for user backends - IUserBackend #12956

Merged

DeepDiver1975 merged commit 6da33e1 into master Dec 19, 2014

DeepDiver1975 deleted the user-backend-names branch December 19, 2014 12:16

lock bot locked as resolved and limited conversation to collaborators Aug 14, 2019

introduce names for user backends - via IUserBackend #12819

introduce names for user backends - via IUserBackend #12819

Conversation

MorrisJobke commented Dec 12, 2014

MorrisJobke commented Dec 12, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MorrisJobke commented Dec 12, 2014

PVince81 commented Dec 15, 2014

PVince81 commented Dec 15, 2014

MorrisJobke commented Dec 15, 2014

PVince81 commented Dec 15, 2014

LukasReschke commented Dec 15, 2014

MorrisJobke commented Dec 16, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickvergessen commented Dec 16, 2014

MorrisJobke commented Dec 16, 2014

nickvergessen commented Dec 16, 2014

MorrisJobke commented Dec 16, 2014

DeepDiver1975 commented Dec 18, 2014

MorrisJobke commented Dec 18, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blizzz commented Dec 18, 2014

MorrisJobke commented Dec 18, 2014

blizzz commented Dec 18, 2014

MorrisJobke commented Dec 18, 2014

Choose a reason for hiding this comment

MorrisJobke commented Dec 18, 2014

MorrisJobke commented Dec 19, 2014

MorrisJobke commented Dec 19, 2014

MorrisJobke commented Dec 19, 2014

MorrisJobke commented Dec 19, 2014

MorrisJobke commented Dec 19, 2014

scrutinizer-notifier commented Dec 19, 2014

ghost commented Dec 19, 2014

Build result: FAILURE

blizzz commented Dec 19, 2014