Fix the group selector drop down #33442

sharidas · 2018-11-06T14:58:23Z

This change gets the group selector drop
down

Signed-off-by: Sujith H [email protected]

Description

Fix the group selector in the core. After moving the code to user_management app, the ajax request to get the groups available for the user did not worked. The reason for this is there is no controller in the core to address the request. This PR fixes the issue.

Related Issue

Fixes "Exclude groups from sharing" checkbox broken on master branch #31739

Motivation and Context

Fix the group selector in the core. After moving the code to user_management app the ajax request to get the groups available for user did not worked in the master branch of core. There was no route and controller to address the request. This PR tries to fix the issue by adding a new route and controller to core which accepts the request and provides the response back.

How Has This Been Tested?

Login as admin and create users group1, group2
Tick the exclude shares check box mentioned in the issue
Admin user can see admin, group1 and group2 listed.

Screenshots (if appropriate):

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)
Technical debt
Tests

Checklist:

Code changes
Unit tests added
Acceptance tests added
Documentation ticket raised:

Open tasks:

Backport (if applicable set "backport-request" label and remove when the backport was done)

sharidas · 2018-11-06T15:06:23Z

Current state of the PR:

Added MetaData.php from user_management to core
Created a new controller for the group selector request.
Update the unit tests

DeepDiver1975 · 2018-11-06T15:33:51Z

settings/Controller/GroupsSelectController.php

+	 * @param int $sortGroups
+	 * @return DataResponse
+	 */
+	public function index($pattern = '', $filterGroups = false, $sortGroups = MetaData::SORT_USERCOUNT) {


you need all parameters here?

DeepDiver1975 · 2018-11-06T15:34:50Z

lib/private/MetaData.php

+
+use OCP\IUserSession;
+
+class MetaData {


As explained I don't want to see this class in code

sharidas · 2018-11-07T09:15:32Z

Current state of the PR:

Added separate controller for group selector
This PR has no dependency with user_management. Meaning, user_management remains untouched.
Added unit test for the change made.

codecov · 2018-11-07T09:40:10Z

Codecov Report

Merging #33442 into master will increase coverage by 0.01%.
The diff coverage is 85%.

@@             Coverage Diff              @@
##             master   #33442      +/-   ##
============================================
+ Coverage     64.22%   64.23%   +0.01%     
- Complexity    18269    18278       +9     
============================================
  Files          1193     1194       +1     
  Lines         69076    69116      +40     
  Branches       1277     1277              
============================================
+ Hits          44363    44397      +34     
- Misses        24341    24347       +6     
  Partials        372      372

Flag	Coverage Δ	Complexity Δ
#javascript	`52.91% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`65.54% <85%> (+0.01%)`	`18278 <9> (+9)`	⬆️

Impacted Files	Coverage Δ	Complexity Δ
settings/routes.php	`100% <ø> (ø)`	`0 <0> (ø)`	⬇️
settings/Application.php	`55.27% <28.57%> (-1.22%)`	`34 <1> (+1)`
settings/Controller/GroupsController.php	`96.96% <96.96%> (ø)`	`8 <8> (?)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f6d3396...229f014. Read the comment docs.

settings/Controller/GroupsController.php

tests/Settings/Controller/GroupsControllerTest.php

sharidas · 2018-11-13T11:13:37Z

@DeepDiver1975 kindly review my change. The updated PR includes:

Updated the message when the user is null
Added missing PHP Doc ( for both controller and the test )
Updated the get request with NoCSRFRequired annotation. Grepped through the source code, and figured out that the GET requests are using the same. Hence adapting it.
Updated the code to use getDisplayName for the group name ( earlier getGID was used ).
Updated the code to return IGroup[] ( earlier it was array|IGroup[] )

PVince81 · 2018-11-14T16:08:35Z

@sharidas mind adding a summary at the top to explain why this PR fixes the issue ?

I see a lot of controller stuff being shuffled around but it's not clear why this helps fixing the issue.

sharidas · 2018-11-14T16:46:22Z

@PVince81

@sharidas mind adding a summary at the top to explain why this PR fixes the issue ?

I see a lot of controller stuff being shuffled around but it's not clear why this helps fixing the issue.

I have updated the Description and Motivation and Context section of the PR. A breif summary has been added to know why this change is added.

PVince81 · 2018-11-15T10:04:53Z

@sharidas is this a brand new controller written from scratch or does it reuse some of the code that was moved to the user_management app ?

sharidas · 2018-11-15T14:16:24Z

@sharidas is this a brand new controller written from scratch or does it reuse some of the code that was moved to the user_management app ?

@PVince81 Yes this is a new controller. It is inspired or lets say influenced from user_management's https://github.com/owncloud/user_management/blob/master/lib/Controller/GroupsController.php. Here is the comment, which helped to create this PR owncloud-archive/user_management#96 (comment). Also I have tried to avoid code duplication while writing this controller. Hope this address your query.

PVince81

See comment.
Looks ok otherwise.

settings/Controller/GroupsController.php

sharidas · 2018-11-16T13:34:21Z

Current state of the PR

Replaced admin users count from 1 to $group->count(''). This was from the request Fix the group selector drop down #33442 (comment). I have verified with the latest change, the code does grab all the users from admin or non admin groups.

Hence kindly request for review @PVince81

tests/Settings/Controller/GroupsControllerTest.php

sharidas · 2018-11-16T15:29:17Z

Updated the test with more than one user in a group.
- Though there is a test which I updated in this version, I have tried to add another test. In the new test I have not used mocks. It directly alters the db.

@PVince81 I have updated the new version with the unit tests updated. I missed updating the test, in my previous version :(

Fix the group selector drop down Signed-off-by: Sujith H <[email protected]>

PVince81 · 2018-11-22T15:45:17Z

@sharidas can you click "resolve conversation" on the review items that you solved ?

sharidas · 2018-11-23T06:16:05Z

@sharidas can you click "resolve conversation" on the review items that you solved ?

I have clicked "resolve conversation" on the review items, that were solved with respect to the current state of the PR.

phil-davis · 2019-03-29T11:53:02Z

Note: This PR would need to be backported to stable10 if we some-day release the user_management app as a separate app, along with a core 10.* release.

sharidas self-assigned this Nov 6, 2018

sharidas added the 2 - Developing label Nov 6, 2018

sharidas added this to the development milestone Nov 6, 2018

sharidas mentioned this pull request Nov 6, 2018

"Exclude groups from sharing" checkbox broken on master branch #31739

Closed

DeepDiver1975 requested changes Nov 6, 2018

View reviewed changes

sharidas mentioned this pull request Nov 6, 2018

[WIP] Move MetaData and Group selector to core owncloud-archive/user_management#96

Closed

sharidas force-pushed the group-selector-fix branch from f3d289f to 8c5ae6d Compare November 7, 2018 09:09

sharidas changed the title ~~[WIP] Fix the group selector drop down~~ Fix the group selector drop down Nov 7, 2018

sharidas added 3 - To Review and removed 2 - Developing labels Nov 7, 2018

sharidas force-pushed the group-selector-fix branch from 8c5ae6d to 6c7db8d Compare November 7, 2018 09:39

DeepDiver1975 requested changes Nov 12, 2018

View reviewed changes

sharidas force-pushed the group-selector-fix branch from 6c7db8d to a874d2d Compare November 13, 2018 10:57

PVince81 reviewed Nov 15, 2018

View reviewed changes

settings/Controller/GroupsController.php Outdated Show resolved Hide resolved

sharidas force-pushed the group-selector-fix branch from a874d2d to efb54cb Compare November 16, 2018 13:29

PVince81 reviewed Nov 16, 2018

View reviewed changes

tests/Settings/Controller/GroupsControllerTest.php Outdated Show resolved Hide resolved

sharidas force-pushed the group-selector-fix branch from efb54cb to d42fa7e Compare November 16, 2018 15:24

sharidas force-pushed the group-selector-fix branch 3 times, most recently from b34a17a to e811ad4 Compare November 19, 2018 07:23

sharidas force-pushed the group-selector-fix branch 2 times, most recently from 42379f9 to bd9aa79 Compare November 20, 2018 15:16

Fix the group selector drop down

229f014

Fix the group selector drop down Signed-off-by: Sujith H <[email protected]>

sharidas force-pushed the group-selector-fix branch from bd9aa79 to 229f014 Compare November 21, 2018 04:30

DeepDiver1975 approved these changes Nov 23, 2018

View reviewed changes

DeepDiver1975 merged commit 1111586 into master Nov 23, 2018

DeepDiver1975 deleted the group-selector-fix branch November 23, 2018 08:07

phil-davis added the only-in-master label Mar 29, 2019

lock bot locked as resolved and limited conversation to collaborators Apr 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the group selector drop down #33442

Fix the group selector drop down #33442

sharidas commented Nov 6, 2018 •

edited

Loading

sharidas commented Nov 6, 2018 •

edited

Loading

DeepDiver1975 Nov 6, 2018

DeepDiver1975 Nov 6, 2018

sharidas commented Nov 7, 2018

codecov bot commented Nov 7, 2018 •

edited

Loading

sharidas commented Nov 13, 2018

PVince81 commented Nov 14, 2018

sharidas commented Nov 14, 2018

PVince81 commented Nov 15, 2018

sharidas commented Nov 15, 2018

PVince81 left a comment

sharidas commented Nov 16, 2018

sharidas commented Nov 16, 2018

PVince81 commented Nov 22, 2018

sharidas commented Nov 23, 2018

phil-davis commented Mar 29, 2019

Fix the group selector drop down #33442

Fix the group selector drop down #33442

Conversation

sharidas commented Nov 6, 2018 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Open tasks:

sharidas commented Nov 6, 2018 • edited Loading

DeepDiver1975 Nov 6, 2018

Choose a reason for hiding this comment

DeepDiver1975 Nov 6, 2018

Choose a reason for hiding this comment

sharidas commented Nov 7, 2018

codecov bot commented Nov 7, 2018 • edited Loading

Codecov Report

sharidas commented Nov 13, 2018

PVince81 commented Nov 14, 2018

sharidas commented Nov 14, 2018

PVince81 commented Nov 15, 2018

sharidas commented Nov 15, 2018

PVince81 left a comment

Choose a reason for hiding this comment

sharidas commented Nov 16, 2018

sharidas commented Nov 16, 2018

PVince81 commented Nov 22, 2018

sharidas commented Nov 23, 2018

phil-davis commented Mar 29, 2019

sharidas commented Nov 6, 2018 •

edited

Loading

sharidas commented Nov 6, 2018 •

edited

Loading

codecov bot commented Nov 7, 2018 •

edited

Loading