Refactor to separate app code logic from core groups/memb logic + keep groups always in sync for centr_groups feature #138

mrow4a · 2018-03-08T22:31:12Z

The goal of following changes were to:

Refactor:

App code is only to serve user interface, and all interaction with core is through custom group manager (CustomGroupManager class) and GroupInterface (group backend). No DAV (user interface related) component should ever call CORE directly.

Always sync with centr_groups:

App keeps its groups/memberbers in sync with core with any action in the UX (add group/user remove group/user)
Central groups in core will result in separate core groups/membership logic so that core never hits the app code (and thus core is more stable)
Manual checks passed with stable10 in core
Manual checks passed with centr_groups in core
Adjusted unit tests

CLAassistant · 2018-03-08T22:31:18Z

All committers have signed the CLA.

mrow4a · 2018-03-08T22:32:52Z

lib/Controller/PageController.php

 		if (is_null($groupInfo)) {
+			// TODO makes no sense, l10n does not exist here..


Should I add l10n?

mrow4a · 2018-03-08T22:33:07Z

lib/Controller/PageController.php

@@ -92,19 +85,21 @@ public function index() {
 	/**
 	 * Search in all groups the current user is member of
 	 *
-	 * @param string $customGroupId custom group id
+	 * TODO: Get explanation what this was supposed to do


I have no idea what is this function about

mrow4a · 2018-03-08T22:33:32Z

lib/CustomGroupsBackend.php

@@ -47,42 +46,13 @@ public function __construct(
 	}

 	/**
-	 * Checks if backend implements actions.
+	 * Expose to the core only group details


This backend exposes to core only groupDetails

mrow4a · 2018-03-08T22:34:27Z

lib/CustomGroupsBackend.php

-	 * @param string $gid gid of the group
-	 * @return boolean true if user is in group, false otherwise
-	 */
-	public function inGroup($uid, $gid) {


all memberships in this backend are gone to core - only groups are kept for consistency.

mrow4a · 2018-03-15T21:20:34Z

Hey @IljaN @PVince81 could I have comments on the justification of change and review of the code? I updated the description

Thanks!

mrow4a · 2018-03-15T21:22:28Z

lib/Dav/GroupMembershipCollection.php

-			throw new NotFound("Group not found \"$groupId\"");
-		}
-		$group->delete();
+		$this->helper->deleteGroup($uri);


Moved the delete group logic to Helper (which does the sync to core also)

mrow4a · 2018-03-15T21:22:40Z

lib/Dav/GroupMembershipCollection.php

-
-		$event = new GenericEvent(null, ['groupName' => $this->groupInfo['display_name'], 'user' => $userId]);
-		$this->dispatcher->dispatch('\OCA\CustomGroups::addUserToGroup', $event);
+		$this->helper->addUser($this->groupInfo['uri'], $userId);


Moved the add user to group logic to Helper (which does the sync to core also)

mrow4a · 2018-03-15T21:22:55Z

lib/Dav/GroupMembershipCollection.php

-		if (!$this->helper->isUserAdmin($this->groupInfo['group_id'])) {
-			return 403;
-		}
+		$result = $this->helper->updateGroup($this->groupInfo['uri'], $displayName);


Moved the group update logic to Helper (which does the sync to core also)

mrow4a · 2018-03-15T21:23:19Z

lib/Dav/GroupsCollection.php

-
-		// add current user as admin
-		$this->groupsHandler->addToGroup($this->helper->getUserId(), $groupId, true);
+		$this->helper->createGroup($name, $displayName);


Moved the group create logic to Helper (which does the sync to core also)

mrow4a · 2018-03-15T21:23:34Z

lib/Dav/MembershipNode.php

-			$event = new GenericEvent(null, ['userId' => $userId, 'groupName' => $this->groupInfo['display_name']]);
-			$this->dispatcher->dispatch('\OCA\CustomGroups::leaveFromGroup', $event);
-		}
+		$this->helper->removeUser($this->groupInfo['uri'], $this->memberInfo['user_id']);


Moved the add group user logic to Helper (which does the sync to core also)

mrow4a · 2018-03-15T22:08:36Z

Keep note that I also keep custom groups and memberships table - this is to not affect negatively stability of the app with big rewrite (with core mem/groups being new), and additionaly for consistency so if something goes really wrong, some cronjob sync will adjust it back (source of truth is custom groups app).

IljaN

👍 for code-style. I don`t have enough knowledge in this part of core to do a full review. So only a nitpick:

If possible and in the scope of this task try to decompose the Helper class (ideally remove it) and fit the methods somewhere else in the architecture (maybe in smaller specialized classes) . Or if a helper method is just used once move it to a private method of the using class.

In my experience helper classes tend to accumulate all kinds of cruft, especially in bigger projects and finally transform to dumps for methods when people are in a hurry or don`t understand the architecture.

https://rrees.me/2009/02/09/the-helper-anti-pattern/
https://en.wikipedia.org/wiki/God_object

PVince81 · 2018-03-16T13:23:16Z

Mind pointing to the changes you did after moving the code ? Another alternative is to have the first PR commit only move the code and the second commit make the change so we can clearly see it. When moving code + changing it is difficult to see what's changed.

Will this code also work with previous 10.0 versions ?

mrow4a · 2018-03-16T13:29:05Z

@PVince81 Good idea! Will do :>

Yes. it will work only with central_groups branch. However, without this change custom groups will also work, but users will see their memberships and new groups on login or when sync cron runs.

So yes, this PR in case of custom groups is required, since it is OC user creating groups, not LDAP admin.

codecov-io · 2018-03-30T15:17:55Z

Codecov Report

Merging #138 into master will increase coverage by 1.3%.
The diff coverage is 99.13%.

@@             Coverage Diff             @@
##             master     #138     +/-   ##
===========================================
+ Coverage     83.61%   84.92%   +1.3%     
- Complexity      300      306      +6     
===========================================
  Files            21       22      +1     
  Lines           952      975     +23     
===========================================
+ Hits            796      828     +32     
+ Misses          156      147      -9

Impacted Files	Coverage Δ	Complexity Δ
lib/CustomGroupsDatabaseHandler.php	`92.48% <100%> (-0.17%)`	`32 <2> (-2)`
lib/Controller/PageController.php	`90% <100%> (+0.11%)`	`32 <0> (ø)`	⬇️
lib/Dav/MembershipNode.php	`96.36% <100%> (-0.66%)`	`24 <0> (-2)`
lib/Dav/UsersCollection.php	`93.93% <100%> (ø)`	`14 <0> (ø)`	⬇️
lib/Dav/GroupsCollection.php	`100% <100%> (+1.58%)`	`23 <0> (ø)`	⬇️
lib/Dav/GroupMembershipCollection.php	`97.53% <100%> (+0.65%)`	`39 <0> (-1)`	⬇️
lib/Service/Helper.php	`100% <100%> (ø)`	`26 <2> (?)`
lib/CustomGroupsBackend.php	`91.22% <100%> (+8.97%)`	`21 <4> (-3)`	⬇️
lib/SettingsPanel.php	`100% <100%> (ø)`	`4 <1> (ø)`	⬇️
lib/Dav/RootCollection.php	`100% <100%> (ø)`	`1 <0> (ø)`	⬇️
... and 3 more

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 5daa016...8b19bf5. Read the comment docs.

mrow4a · 2018-03-30T15:31:28Z

@PVince81 @IljaN @tomneedham @butonic Just tested this app both with unit testss (>90% coverage) and manualy for stable10 core with 1st commit. This refactor should make this app cleaner, @IljaN thanks for earlier comments, they were very helpful - now there is a class CustomGroupsManager.

mrow4a · 2018-03-30T15:36:52Z

Yes, after change in first commit, this app will work with both central groups and with earlier versions. This is due to the fact I really separated this app from core.. the only difference with central_groups is that instead core hitting this app for groups (through GroupInterface), it will not touch it and use central groups table. On other hand, app will sync all its changes with core (except admin of custom groups, since core does not care and this is only UX feature) @PVince81 @DeepDiver1975

mrow4a · 2018-03-30T16:51:45Z

appinfo/info.xml

@@ -37,7 +37,7 @@ Sharing with a Custom Group is as easy and quick as always. Just click on the sh
 	</types>
 	<use-migrations>true</use-migrations>
 	<dependencies>
-		<owncloud min-version="10.0.3" max-version="10.1" />
+		<owncloud min-version="10.1" max-version="10.1" />


@PVince81 ok, with second commit I needed to increase the requirement, since createGroupFromBackend is needed for syncing with core. Everything else works as previously.

First commit however passed all checks with stable core

mrow4a · 2020-01-14T18:33:06Z

central group tables wont be done

mrow4a self-assigned this Mar 8, 2018

mrow4a requested review from PVince81, tomneedham, butonic, DeepDiver1975 and sharidas March 8, 2018 22:31

mrow4a commented Mar 8, 2018

View reviewed changes

mrow4a force-pushed the centr_groups branch from 00f56a9 to 36968e4 Compare March 15, 2018 21:15

mrow4a changed the title ~~[WIP] Adjust custom groups to central group table~~ Separate app code logic from core groups/memb logic + keep groups always in sync Mar 15, 2018

mrow4a force-pushed the centr_groups branch from 36968e4 to 5c5c44a Compare March 15, 2018 21:19

mrow4a commented Mar 15, 2018

View reviewed changes

mrow4a force-pushed the centr_groups branch from 5c5c44a to 4c514ca Compare March 15, 2018 21:52

IljaN reviewed Mar 16, 2018

View reviewed changes

mrow4a force-pushed the centr_groups branch from 4c514ca to c3d398b Compare March 30, 2018 15:01

mrow4a changed the title ~~Separate app code logic from core groups/memb logic + keep groups always in sync~~ Refactor to separate app code logic from core groups/memb logic + keep groups always in sync for centr_groups feature Mar 30, 2018

mrow4a force-pushed the centr_groups branch from c3d398b to 716c716 Compare March 30, 2018 15:06

Refactor to separate app code logic from core groups/membership logic

9accee1

mrow4a force-pushed the centr_groups branch from 716c716 to 9accee1 Compare March 30, 2018 15:17

Sync with core the changes in this app

8b19bf5

mrow4a commented Mar 30, 2018

View reviewed changes

mrow4a mentioned this pull request May 6, 2018

Central membership and group table owncloud/core#29107

Closed

24 tasks

mrow4a closed this Jan 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor to separate app code logic from core groups/memb logic + keep groups always in sync for centr_groups feature #138

Refactor to separate app code logic from core groups/memb logic + keep groups always in sync for centr_groups feature #138

mrow4a commented Mar 8, 2018 •

edited

Loading

CLAassistant commented Mar 8, 2018 •

edited

Loading

mrow4a Mar 8, 2018

mrow4a Mar 8, 2018

mrow4a Mar 8, 2018

mrow4a Mar 8, 2018 •

edited

Loading

mrow4a commented Mar 15, 2018

mrow4a Mar 15, 2018

mrow4a Mar 15, 2018

mrow4a Mar 15, 2018

mrow4a Mar 15, 2018

mrow4a Mar 15, 2018

mrow4a commented Mar 15, 2018 •

edited

Loading

IljaN left a comment •

edited

Loading

PVince81 commented Mar 16, 2018

mrow4a commented Mar 16, 2018 •

edited

Loading

codecov-io commented Mar 30, 2018 •

edited

Loading

mrow4a commented Mar 30, 2018 •

edited

Loading

mrow4a commented Mar 30, 2018 •

edited

Loading

mrow4a Mar 30, 2018

mrow4a Mar 30, 2018

mrow4a commented Jan 14, 2020

		if (is_null($groupInfo)) {
		// TODO makes no sense, l10n does not exist here..

Refactor to separate app code logic from core groups/memb logic + keep groups always in sync for centr_groups feature #138

Refactor to separate app code logic from core groups/memb logic + keep groups always in sync for centr_groups feature #138

Conversation

mrow4a commented Mar 8, 2018 • edited Loading

CLAassistant commented Mar 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrow4a Mar 8, 2018 • edited Loading

Choose a reason for hiding this comment

mrow4a commented Mar 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrow4a commented Mar 15, 2018 • edited Loading

IljaN left a comment • edited Loading

Choose a reason for hiding this comment

PVince81 commented Mar 16, 2018

mrow4a commented Mar 16, 2018 • edited Loading

codecov-io commented Mar 30, 2018 • edited Loading

Codecov Report

mrow4a commented Mar 30, 2018 • edited Loading

mrow4a commented Mar 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrow4a commented Jan 14, 2020

mrow4a commented Mar 8, 2018 •

edited

Loading

CLAassistant commented Mar 8, 2018 •

edited

Loading

mrow4a Mar 8, 2018 •

edited

Loading

mrow4a commented Mar 15, 2018 •

edited

Loading

IljaN left a comment •

edited

Loading

mrow4a commented Mar 16, 2018 •

edited

Loading

codecov-io commented Mar 30, 2018 •

edited

Loading

mrow4a commented Mar 30, 2018 •

edited

Loading

mrow4a commented Mar 30, 2018 •

edited

Loading