Remove addgroup from user #40770

jvillafanez · 2023-05-03T11:41:57Z

Description

This PR includes 3 changes:

Remove the "add group" button from the dropdowns. New groups can be added from the "add group" button in the top left side of the page. The dropdowns will only be used to select or deselect groups.
Fix issue with the group count in the left sidebar when a new group was created (group count still shows only if ldap is disabled)
Use group's displayname in more places in the web UI

The 3rd point should be mostly transparent. The places where the displayname wasn't used are mostly pointing to local groups, whose id and displayname matches. There are still a couple of messages using the group id, but those will require more changes.

Related Issue

https://github.com/owncloud/enterprise/issues/5684

Motivation and Context

The "add group" button was removed because the behavior could cause confusion, specially when creating a new user: the groups weren't created until the user was created, but they appear as such in the web UI.

The changes in the displayname shouldn't be noticed, but the web UI should be more prepared now

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Database schema changes (next release will require increase of minor version instead of patch)
Breaking change (fix or feature that would cause existing functionality to change)
Technical debt
Tests only (no source changes)

Checklist:

update-docs · 2023-05-03T11:42:01Z

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

ownclouders · 2023-05-03T15:38:42Z

💥 Acceptance tests pipeline apiWebdavLocksUnlock-maria10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/38379/84

ownclouders · 2023-05-03T15:38:42Z

💥 Acceptance tests pipeline apiFederationToRoot1-latest-maria10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/38379/97

ownclouders · 2023-05-03T15:38:42Z

💥 Acceptance tests pipeline apiWebdavMove2-maria10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/38379/86

ownclouders · 2023-05-03T15:38:42Z

💥 Acceptance tests pipeline apiWebdavUpload1-maria10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/38379/91

ownclouders · 2023-05-03T15:38:42Z

💥 Acceptance tests pipeline apiSharingNotificationsToRoot-maria10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/38379/94

jvillafanez · 2023-05-04T10:47:44Z

In order to check the changes for the displayname, you can use the following patch (*)

diff --git a/lib/private/Group/Database.php b/lib/private/Group/Database.php
index 4b8eb79de3..daf946583e 100644
--- a/lib/private/Group/Database.php
+++ b/lib/private/Group/Database.php
@@ -349,4 +349,11 @@ class Database extends \OC\Group\Backend {
                }
                return $count;
        }
+
+       public function getGroupDetails($gid) {
+               return [
+                       'gid' => $gid,
+                       'displayName' => "{$gid}__{$gid}",
+               ];
+       }
 }

(*) This is intended to be used just for testing. DO NOT use it on production. Right now, it isn't possible to use a displayname for local groups, and there is no short term plan for this feature.

sonarqubecloud · 2023-05-04T11:04:45Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
15 Code Smells

24.5% Coverage
0.0% Duplication

jvillafanez · 2023-05-04T13:27:40Z

There are no tests for js files, and the ajax part can't be unit tested, so it isn't possible to rise the code coverage.

pako81 · 2023-07-31T14:06:09Z

It would be nice to also have this in 10.13. @phil-davis @jnweiger what to do here?

phil-davis · 2023-07-31T14:35:41Z

It would be nice to also have this in 10.13. @phil-davis @jnweiger what to do here?

IMO this is "a good thing". I don't think that it is really necessary to be able to add a group on-the-fly while adding a user. Groups can be easily created "separately", which is how I imagine it is done 99% of the time anyway. Removing the "add group" stuff from inside the "add user" UI means we can avoid all the edge cases for what happens when the admin has got to various points in the UI workflow, then aborted, or "something happened" when trying to save the user (and new group along with it)...

We just need a developer review. Who do we mention for reviews of PRs done by @jvillafanez ?

pako81 · 2023-07-31T14:38:42Z

We just need a developer review. Who do we mention for reviews of PRs done by @jvillafanez ?

Had a look at the code myself and it looks ok. Still it would be good if someone else could review this. @mrow4a or @steelcuts maybe?

phil-davis · 2023-07-31T14:41:41Z

Note: existing UI automated acceptance tests cover adding a group from the "normal" left-hand side of the admin "users page". They also cover adding a user, including selecting existing groups that the user will become a member of. CI passes, so the behavior of all that "ordinary workflow: still works.

We never had automated acceptance tests that created a group on-the-fly while creating a user. So that is why there are no acceptance tests to remove when removing that functionality.

cdamken · 2023-07-31T14:48:14Z

Had a look at the code myself and it looks ok. Still it would be good if someone else could review this. @mrow4a or @steelcuts maybe?

I asked both if they can provide a review.

mrow4a

minor clarifications, plus what do we do with Sonar?

mrow4a · 2023-08-01T07:12:17Z

settings/js/users/groups.js

 			var $li = $(
 				'<li class="isgroup" data-gid="' + gid + '" data-usercount="0">' +
 				'	<a href="#" class="dorename">' +
-				'		<span class="groupname">' + gid + '</span>' +
+				'		<span class="groupname">' + name + '</span>' +


settings/Controller/GroupsController.php

settings/js/users/users.js

jvillafanez · 2023-08-01T09:48:08Z

I've replaced the "id" to "gid". I've also added some changes in the last commit in "settings/js/users/users.js" around lines 737 and 759 in order to retrieve the group info, which seemed to give some problems in some scenarios (the previous behavior might be okish, but there were some errors in the console).

phil-davis · 2023-08-01T10:01:36Z

https://drone.owncloud.com/owncloud/core/38803
This branch has an out-of-date .drone.star
This branch is 8 commits ahead, 244 commits behind master.

Needs a rebase so that CI can run.

pako81 · 2023-08-01T21:01:34Z

Acceptance tests are failing with Page\UsersPage::waitTillPageIsLoaded timeout waiting for user list to load on users page (Exception) --> https://github.com/owncloud/core/blob/master/tests/acceptance/features/lib/UsersPage.php#L982

phil-davis · 2023-08-02T05:44:47Z

The UI acceptance tests run with an old version of Chrome browser. That is because of issues with the Behat-Mink-selenium test framework, it is not able to control newer versions of Chrome or Firefox that have a newer interface for automated testing.
We have not pushed to try and fix this upstream, because the Behat-Mink-Selenium stack is only being used for automated UI testing of the classic web UI, and that does not have much change happening.

But it means that new JS features cannot be used. In this case, the "null coalescing operator" ?? is a "new" JavaScript thing. It is not valid in the old version of Chrome used in the automated tests.

I changed the syntax back from x ?? y to x ? x : y syntax. See the recent commit that I pushed. The tests should pass.

sonarqubecloud · 2023-08-02T06:03:35Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
27 Code Smells

24.2% Coverage
0.0% Duplication

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

phil-davis · 2023-08-02T07:16:28Z

There are no tests for js files, and the ajax part can't be unit tested, so it isn't possible to rise the code coverage.

Note: we know why SonarCloud is complaining about the code coverage.

IMO this can be merged - drone CI passed.

jvillafanez added the 3 - To Review label May 3, 2023

jvillafanez self-assigned this May 3, 2023

phil-davis requested review from mrow4a and steelcuts July 31, 2023 14:51

mrow4a reviewed Aug 1, 2023

View reviewed changes

jvillafanez added 8 commits August 1, 2023 15:16

Remove add group option from the dropdown menus

5518dd6

Fix user count after adding a new user to a group

1aa0928

Use the group's displayname in more places

4f45ee8

Homogenize group data

356a0b8

Fix tests and adjust code

49f67a0

Increase coverage

54dc378

Add changelog entry

bac601b

Replace id to gid

fa8c662

jvillafanez force-pushed the remove_addgroup_from_user branch from d176d41 to fa8c662 Compare August 1, 2023 13:16

mrow4a approved these changes Aug 1, 2023

View reviewed changes

Do not use null coalescing operator in JS

3e6e002

phil-davis approved these changes Aug 2, 2023

View reviewed changes

pako81 merged commit f525078 into master Aug 2, 2023

delete-merged-branch bot deleted the remove_addgroup_from_user branch August 2, 2023 07:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove addgroup from user #40770

Remove addgroup from user #40770

jvillafanez commented May 3, 2023

update-docs bot commented May 3, 2023

ownclouders commented May 3, 2023

ownclouders commented May 3, 2023

ownclouders commented May 3, 2023

ownclouders commented May 3, 2023

ownclouders commented May 3, 2023

jvillafanez commented May 4, 2023

sonarqubecloud bot commented May 4, 2023

jvillafanez commented May 4, 2023

pako81 commented Jul 31, 2023 •

edited

Loading

phil-davis commented Jul 31, 2023

pako81 commented Jul 31, 2023 •

edited

Loading

phil-davis commented Jul 31, 2023 •

edited

Loading

cdamken commented Jul 31, 2023

mrow4a left a comment •

edited

Loading

mrow4a Aug 1, 2023

jvillafanez commented Aug 1, 2023

phil-davis commented Aug 1, 2023

pako81 commented Aug 1, 2023

phil-davis commented Aug 2, 2023

sonarqubecloud bot commented Aug 2, 2023

phil-davis commented Aug 2, 2023

Remove addgroup from user #40770

Remove addgroup from user #40770

Conversation

jvillafanez commented May 3, 2023

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

update-docs bot commented May 3, 2023

ownclouders commented May 3, 2023

ownclouders commented May 3, 2023

ownclouders commented May 3, 2023

ownclouders commented May 3, 2023

ownclouders commented May 3, 2023

jvillafanez commented May 4, 2023

sonarqubecloud bot commented May 4, 2023

jvillafanez commented May 4, 2023

pako81 commented Jul 31, 2023 • edited Loading

phil-davis commented Jul 31, 2023

pako81 commented Jul 31, 2023 • edited Loading

phil-davis commented Jul 31, 2023 • edited Loading

cdamken commented Jul 31, 2023

mrow4a left a comment • edited Loading

Choose a reason for hiding this comment

mrow4a Aug 1, 2023

Choose a reason for hiding this comment

jvillafanez commented Aug 1, 2023

phil-davis commented Aug 1, 2023

pako81 commented Aug 1, 2023

phil-davis commented Aug 2, 2023

sonarqubecloud bot commented Aug 2, 2023

phil-davis commented Aug 2, 2023

pako81 commented Jul 31, 2023 •

edited

Loading

pako81 commented Jul 31, 2023 •

edited

Loading

phil-davis commented Jul 31, 2023 •

edited

Loading

mrow4a left a comment •

edited

Loading