Full implement group display names by nickvergessen · Pull Request #8255 · nextcloud/server

nickvergessen · 2018-02-08T08:54:04Z

Personal settings show group id instead of display names in list
Activities use group id instead of display names
Select2 in the WorkflowEngine shows the id on load, dropdown and selecting new values works
Select2 in the OC.Settings.setupGroupsSelect shows the id on load, dropdown and selecting new values works
User management: user group and subadmin selection is messed up
Tests

Need someone with better JS/select2 skills to solve this.

Good patch for testing

diff --git a/lib/private/Group/Group.php b/lib/private/Group/Group.php
index 6795f0e2f5..24e8a7ad00 100644
--- a/lib/private/Group/Group.php
+++ b/lib/private/Group/Group.php
@@ -87,9 +87,9 @@ class Group implements IGroup {
 
        public function getDisplayName() {
                if (is_null($this->displayName)) {
-                       return $this->gid;
+                       return 'DisplayName{' . $this->gid . '}';
                }
-               return $this->displayName;
+               return 'DisplayName{' . $this->displayName . '}';
        }
 
        /**

Fixes #742

nickvergessen · 2018-02-08T08:54:29Z

@nextcloud/javascript who could help?

MorrisJobke · 2018-02-26T13:27:52Z

@nextcloud/javascript who could help?

maybe @danxuliu or @skjnldsv

codecov · 2018-03-02T14:22:18Z

Codecov Report

Merging #8255 into master will decrease coverage by 16.87%.
The diff coverage is 4.91%.

@@              Coverage Diff              @@
##             master    #8255       +/-   ##
=============================================
- Coverage     51.87%   34.99%   -16.88%     
+ Complexity    25422    25376       -46     
=============================================
  Files          1609     1607        -2     
  Lines         95343    95159      -184     
  Branches       1378     1378               
=============================================
- Hits          49455    33297    -16158     
- Misses        45888    61862    +15974

Impacted Files	Coverage Δ	Complexity Δ
settings/templates/users/part.grouplist.php	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
apps/dav/lib/CalDAV/Activity/Provider/Base.php	`0% <0%> (-76.79%)`	`19 <7> (+3)`
apps/dav/lib/CalDAV/Activity/Provider/Calendar.php	`0% <0%> (ø)`	`65 <1> (ø)`	⬇️
settings/templates/users/main.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
lib/private/Group/MetaData.php	`0% <0%> (-77.03%)`	`19 <0> (ø)`
apps/dav/lib/CalDAV/Activity/Provider/Event.php	`0% <0%> (ø)`	`25 <1> (ø)`	⬇️
lib/private/SubAdmin.php	`21.31% <0%> (-74.49%)`	`27 <1> (+1)`
settings/Controller/GroupsController.php	`10% <0%> (-54.62%)`	`9 <0> (ø)`
lib/private/Settings/Personal/PersonalInfo.php	`0% <0%> (ø)`	`30 <0> (ø)`	⬇️
...ps/files_sharing/lib/Activity/Providers/Groups.php	`0% <0%> (ø)`	`22 <5> (+5)`	⬆️
... and 559 more

skjnldsv · 2018-03-02T16:19:11Z

Select2 issue is a big one!
Basically, since we store only the value of the select (aka groupid) and considering the fact that we only retrieve the groups data onclick on the select, the default text can't be anything else than the group id.
One solution would be to create the select as html with all of its options and apply select2 directly on it. I didn't test it, but it should work

skjnldsv · 2018-03-02T17:04:32Z

I'll change my view! I tried a different approach where I fetch the groups on page load only (no need to fetch them on each select2 click).
So I added on handlebar Views an ajax requests that get all the groups and cache them.
Then re-render the full select2. It works perfectly! :)

It quickly displays the id for a short moment, the time to requests the groups, then update itself with the proper displayName!

skjnldsv · 2018-03-03T11:23:26Z

~~I could use a hand on tests fixing for the last tests please @nickvergessen :)~~

All tests fixed

skjnldsv · 2018-03-05T09:34:55Z

Test passes, failure unrelated :)

Please review

nickvergessen · 2018-03-05T11:53:43Z

apps/workflowengine/js/admin.js

+				}).done(function(response) {
+					// add admin groups
+					$.each(response.data.adminGroups, function(id, group) {
+						self.groups.push({ id: group.id, displayname: group.name+'FIXME' });


Still broken

okay, here it works, just need to remove the FIXME

Ahaha oups! 🙈

nickvergessen · 2018-03-05T11:53:48Z

apps/workflowengine/js/admin.js

+					});
+					// add groups
+					$.each(response.data.groups, function(id, group) {
+						self.groups.push({ id: group.id, displayname: group.name+'FIXME' });


Still broken

okay, here it works, just need to remove the FIXME

nickvergessen · 2018-03-05T11:54:11Z

settings/js/settings.js

-							id: groupName,
-							displayname: groupName
+							id: groupId,
+							displayname: groupId + 'FIXME' // FIXME


Still broken (test with "exclude group from sharing")

In file access control or workflow?

AH okay, I see. I did not see it was there :)

in admin> sharing>

nickvergessen · 2018-03-05T12:05:56Z

Creating a new user and also adding them to a group (while creating the user) is broken, send data for groups is corrupted...

email |  
groups[ug-test][displayName] | DisplayName{ug-test}
password | 12346
username | test2e3e

Fixed with 3a9297e

blizzz · 2018-03-05T12:07:00Z

settings/Controller/GroupsController.php

-				),
-				Http::STATUS_CREATED
-			);
+		$group = $this->groupManager->createGroup($id);


there's not method to set the group name? not needed?

No not needed atm

skjnldsv · 2018-03-05T13:51:32Z

apps/workflowengine/js/usergroupmembershipplugin.js

+					if (groupId && groups.length > 0) {
+						callback({
+							id: groupId,
+							displayname: groups.find(group =>group.id === groupId).displayname


Hum, this is ES6, I'm not sure IE11 supports it 🤔

We should not do this yet.

skjnldsv · 2018-03-05T14:54:51Z

search not working anymore

rullzer · 2018-03-07T12:00:10Z

@skjnldsv #8255 (comment)

So I added on handlebar Views an ajax requests that get all the groups and cache them.

does this scale? Like even when you are on ldap and there are a few thousand groups?

skjnldsv · 2018-03-07T12:03:09Z

@rullzer I have no idea! The request was made anyway, I just did it before loading the select2 instead of after initialising it :)

skjnldsv · 2018-03-08T15:49:48Z

Failure related:

Scenario: Getting subadmin groups                               # /drone/src/github.com/nextcloud/server/build/integration/features/provisioning-v1.feature:252
  Given As an "admin"                                           # FeatureContext::asAn()
[Wed Mar  7 09:44:38 2018] 127.0.0.1:53098 [200]: /ocs/v2.php/cloud/users/brand-new-user
[Wed Mar  7 09:44:38 2018] 127.0.0.1:53108 [200]: /ocs/v2.php/cloud/users/brand-new-user
  And user "brand-new-user" exists                              # FeatureContext::assureUserExists()
[Wed Mar  7 09:44:38 2018] 127.0.0.1:53110 [200]: /ocs/v2.php/cloud/groups/new-group
[Wed Mar  7 09:44:38 2018] 127.0.0.1:53114 [200]: /ocs/v2.php/cloud/groups/new-group
  And group "new-group" exists                                  # FeatureContext::assureGroupExists()
[Wed Mar  7 09:44:38 2018] 127.0.0.1:53122 [200]: /ocs/v1.php/cloud/users/brand-new-user/subadmins
  When sending "GET" to "/cloud/users/brand-new-user/subadmins" # FeatureContext::sendingTo()
  Then subadmin groups returned are                             # FeatureContext::theSubadminGroupsShouldBe()
    | new-group |
    Failed asserting that two arrays are equal.
    --- Expected
    +++ Actual
    @@ @@
     Array (
    -    0 => 'new-group'
     )

EDIT: FIXED! 🚀

Signed-off-by: Joas Schilling <coding@schilljs.com>

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

Signed-off-by: Joas Schilling <coding@schilljs.com>

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

skjnldsv · 2018-03-08T16:48:00Z

Failure unrelated: swift

nickvergessen · 2018-03-12T11:28:08Z

Backported the user facing stuff in #8779

blizzz

External storages shows group id, i'm okay if this gets adjusted in a follow up PR though.

rullzer

Lets do this and fix other occurences when we find them

nickvergessen added enhancement help wanted 2. developing Work in progress labels Feb 8, 2018

nickvergessen added this to the Nextcloud 14 milestone Feb 8, 2018

skjnldsv self-assigned this Mar 2, 2018

skjnldsv force-pushed the bugfix/noid/group-display-name branch from a76d28e to 2be5980 Compare March 2, 2018 17:21

skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Mar 5, 2018

skjnldsv requested review from LukasReschke, MorrisJobke, rullzer and skjnldsv March 5, 2018 09:35

skjnldsv added the high label Mar 5, 2018

nickvergessen commented Mar 5, 2018

View reviewed changes

blizzz reviewed Mar 5, 2018

View reviewed changes

skjnldsv reviewed Mar 5, 2018

View reviewed changes

oparoz removed the high label Mar 6, 2018

MorrisJobke mentioned this pull request Mar 6, 2018

Add display name for groups #742

Closed

skjnldsv added 3. to review Waiting for reviews and removed 3. to review Waiting for reviews 2. developing Work in progress labels Mar 8, 2018

nickvergessen and others added 17 commits March 8, 2018 17:13

Show the displayname in the users group list

aef86a5

Signed-off-by: Joas Schilling <coding@schilljs.com>

Correctly return the group name

82137f4

Signed-off-by: Joas Schilling <coding@schilljs.com>

Start fixing Select2 options

0e23662

Signed-off-by: Joas Schilling <coding@schilljs.com>

Fix group displaynames in activity

db9b239

Signed-off-by: Joas Schilling <coding@schilljs.com>

Fix group display name in the sidebar of the user management

cb2fbdd

Signed-off-by: Joas Schilling <coding@schilljs.com>

Use group displayNames in users list

00122fa

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

Fix users loading on group click and group creation on select

ddcd371

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

Fixed select2 workflow

e6a7a9f

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

Fixed Controller Test

9440e9b

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

Fix Metadata tests

d80f13e

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

Fixed caldav tests and metadata 2

720e64b

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

Remove resolved FIXME

1f9a5c0

Signed-off-by: Joas Schilling <coding@schilljs.com>

Send the group ids to the server on user creation

51ec928

Signed-off-by: Joas Schilling <coding@schilljs.com>

Fixed sharing groups select and fixed search

6e0044b

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

fixup! Fixed sharing groups select and fixed search

d435c91

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

No ES6 :(

f1478ea

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

Fixed api tests

23a1553

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

skjnldsv force-pushed the bugfix/noid/group-display-name branch from 4a2818b to 23a1553 Compare March 8, 2018 16:13

nickvergessen mentioned this pull request Mar 12, 2018

[stable13] Show group display names #8779

Merged

blizzz approved these changes Mar 13, 2018

View reviewed changes

rullzer approved these changes Mar 15, 2018

View reviewed changes

blizzz merged commit 208e38e into master Mar 15, 2018

blizzz deleted the bugfix/noid/group-display-name branch March 15, 2018 11:07

skjnldsv mentioned this pull request Mar 17, 2018

Return groups displayname in provisioning api #8865

Merged

MorrisJobke mentioned this pull request Apr 17, 2018

Rename group display names #9209

Closed

Uh oh!

Conversation

nickvergessen commented Feb 8, 2018 • edited by MorrisJobke Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Good patch for testing

Uh oh!

nickvergessen commented Feb 8, 2018

Uh oh!

MorrisJobke commented Feb 26, 2018

Uh oh!

codecov bot commented Mar 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

skjnldsv commented Mar 2, 2018

Uh oh!

skjnldsv commented Mar 2, 2018

Uh oh!

skjnldsv commented Mar 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skjnldsv commented Mar 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nickvergessen commented Mar 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skjnldsv commented Mar 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rullzer commented Mar 7, 2018

Uh oh!

skjnldsv commented Mar 7, 2018

Uh oh!

skjnldsv commented Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skjnldsv commented Mar 8, 2018

Uh oh!

nickvergessen commented Mar 12, 2018

Uh oh!

blizzz left a comment

Choose a reason for hiding this comment

Uh oh!

rullzer left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

nickvergessen commented Feb 8, 2018 •

edited by MorrisJobke

Loading

codecov bot commented Mar 2, 2018 •

edited

Loading

skjnldsv commented Mar 3, 2018 •

edited

Loading

nickvergessen commented Mar 5, 2018 •

edited

Loading

skjnldsv commented Mar 5, 2018 •

edited

Loading

skjnldsv commented Mar 8, 2018 •

edited

Loading