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

+ }).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

+ });
+ // 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

- 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

- ),
- 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

+ 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 2be5980Compare 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

nickvergessenand 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 23a1553Compare 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 masterMar 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!

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!

codecovBot 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

nickvergessen commented Feb 8, 2018•
edited by MorrisJobke
Loading

codecovBot 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