[Central group table] Add membership manager to central group table #29154

mrow4a · 2017-10-05T20:38:36Z

This is first approach to handle central group table feature. It utilizes new class Membership manager, which inherits from Access class. More implementation details will go inside the code.

NOTE: Mind that this PR target is centr_groups branch, from this PR #29107

@patrickjahns

mrow4a · 2017-10-05T20:40:13Z

I will also start writing unit tests to test queries I am using, so dont focus on them to much at that point.

If we decide on that approach, lets merge it with #29107. This should not affect any other class and existing functionalities. It will when all TODOs will be implemented.

mrow4a · 2017-10-05T20:41:00Z

lib/private/Group/Group.php

+	private $backendGroup;
+
+	/**
+	 * // TODO: use BackendGroup here


All the TODOs in the code are places which should be implemented after merging this PR

mrow4a · 2017-10-05T20:56:29Z

lib/private/Group/GroupMapper.php

-		return $this->findEntity($qb->getSQL(), $qb->getParameters());
+		try {
+			/** @var BackendGroup $backendGroup */
+			$backendGroup = $this->findEntity($qb->getSQL(), $qb->getParameters());


Add some additional checks which are not in original version. It should return null and not throw exception

mrow4a · 2017-10-05T20:56:57Z

lib/private/Group/Manager.php

@@ -143,10 +144,21 @@ protected function clearCaches() {

 	/**
 	 * @param string $gid
+	 * @return boolean
+	 */
+	public function isCached($gid) {


This method is required to build optimized queries

public? I don't see any reason to make this function public. This should be private.

True, will write a UserCacheManager and GroupCacheManager Włoch will be used Both in Group/User Manager and Mapper

mrow4a · 2017-10-05T20:57:43Z

lib/private/Server.php

 		});
 		$this->registerService('GroupMapper', function(Server $c) {
 			return new GroupMapper($c->getDatabaseConnection());
 		});
+		$this->registerService('MembershipManager', function(Server $c) {
+			return new MembershipManager($c->getDatabaseConnection(), $c->getConfig(),


All changes in this file are obligatory so it actually all works.

mrow4a · 2017-10-05T21:00:50Z

lib/public/AppFramework/Db/Mapper.php

+	 * @throws \BadFunctionCallException
+	 * @since 7.0.0
+	 */
+	public function mapRowToEntity($row) {


Make it public so MembershipManager can also create the Account entities (otherwise it will only be possible inside AccountMapper)

Added @throws in phpdoc to make it clear that this function should only be used by classes which operate over oc_accounts table.

mrow4a · 2017-10-05T21:01:53Z

lib/private/MembershipManager.php

+use Doctrine\DBAL\Exception\UniqueConstraintViolationException;
+use OCP\DB\QueryBuilder\IQueryBuilder;
+
+class MembershipManager extends Access {


Please have a look on all the public functions of that class. To define them, I used Group, User, GroupManager, UserManager and SubAdmin finding requirements, and placed TODOs in all the places

As said in #29107 (comment) , probably a bad idea to extend from Access

mrow4a · 2017-10-05T21:04:19Z

lib/private/MembershipManager.php

+		// Adjust the query depending on availability of accountId and $backendGroupId
+		// to have optimized access
+		$alias = $this->getTableAlias();
+		if ($this->userManager->isCached($userId) && $this->groupManager->isCached($gid)) {


I use isCached mathods of Managers to build optimized queries - it means, fetch info from cache if available, otherwise perform JOIN.

From my point of view, this reveals a design flaw. Why something related to the DB access has to check a class from the business logic? Why it needs to check if it's cached when noone should be interested in that?.

I think we should recheck the approach. What about the group manager handles also the membership? Could it simplify this?

mrow4a · 2017-10-09T13:05:41Z

@jvillafanez

jvillafanez · 2017-10-10T09:18:12Z

I'll need to do a second review

mrow4a · 2017-10-11T08:36:10Z

I need to have second take on it, it was just poc, trying to figure out how this should be structured, and it is far from proper. Thanks for valuable feedback @jvillafanez, I also take into account your architecture vision.

mrow4a · 2017-11-07T20:44:24Z

@DeepDiver1975 @PVince81 I have tested it with #29282 integration into group manager, group, subadmin, user and user manager. This PR is ready

mrow4a · 2017-11-14T08:07:26Z

Any input here?

jvillafanez · 2017-11-14T17:36:18Z

lib/private/MembershipManager.php

+	const MEMBERSHIP_TYPE_GROUP_ADMIN = 1;
+
+	/** @var IConfig */
+	protected $config;


If this class isn't expected to be extended, this should be private.

DeepDiver1975 · 2017-11-24T07:52:53Z

@ownclouders rebase

ownclouders · 2017-11-24T07:52:54Z

Hey! This pull request is being rebased automatically. Please DO NOT push while rebase is in progress or your changes would be lost!

DeepDiver1975 · 2017-11-24T07:47:12Z

lib/private/MembershipManager.php

+	private $db;
+
+
+	public function __construct(IDBConnection $db, IConfig $config) {


PHPDoc missing

DeepDiver1975 · 2017-11-24T07:48:30Z

lib/private/MembershipManager.php

+	 * the user has specific membership type
+	 *
+	 * @param string|int $userId
+	 * @param bool $isInternalUserId


what is internal?

DeepDiver1975 · 2017-11-24T07:48:37Z

lib/private/MembershipManager.php

+	}
+
+	/*
+	 * Return backend group entities for given user id $userId (internal or gid) of which


DeepDiver1975 · 2017-11-24T07:49:58Z

lib/private/MembershipManager.php

+	 *
+	 * @return BackendGroup[]
+	 */
+	public function getUserBackendGroups($userId) {


do we really need this function? From my understanding everything should internally work on accountIds

It is just optimization, in case account id is only available by fetching the use from user manager.

DeepDiver1975 · 2017-11-24T07:51:38Z

lib/private/Server.php

+	/**
+	 * @return \OC\User\AccountTermMapper
+	 */
+	public function getAccountTermMapper() {


why? public getters in class Server shall only return OCP interfaces

Hmm, so what with AccountMapper?

ownclouders · 2017-11-24T07:53:12Z

Automated rebase was successful!

mrow4a · 2017-11-28T13:04:35Z

@DeepDiver1975 addressed your doubts and fixed

DeepDiver1975 · 2017-11-30T09:17:12Z

tests/lib/MembershipManagerTest.php

+use OC\User\AccountTermMapper;
+use OCP\IConfig;
+use OCP\IDBConnection;
+use Test\TestCase;


DeepDiver1975 · 2017-11-30T09:17:19Z

tests/lib/MembershipManagerTest.php

+use OCP\IConfig;
+use OCP\IDBConnection;
+use Test\TestCase;
+use OCP\AppFramework\Db\DoesNotExistException;


DeepDiver1975 · 2017-11-30T09:18:21Z

tests/lib/MembershipManagerTest.php

+	private static function clearDB() {
+		\OC::$server->getDatabaseConnection()->executeQuery('DELETE FROM `*PREFIX*memberships`');
+		\OC::$server->getDatabaseConnection()->executeQuery('DELETE FROM `*PREFIX*backend_groups`');
+		\OC::$server->getDatabaseConnection()->executeQuery('DELETE FROM `*PREFIX*accounts`');


really kill all?

DeepDiver1975 · 2017-11-30T09:19:09Z

lib/private/Server.php

@@ -89,6 +89,7 @@
 use OCP\AppFramework\QueryException;
 use OCP\AppFramework\Utility\ITimeFactory;
 use OC\Group\GroupMapper;
+use OC\MembershipManager;


DeepDiver1975 · 2017-11-30T09:19:21Z

lib/private/Server.php

-					$c->getMemCacheFactory(),
-					$appManager,
-					$c->getTempManager()
+				new EnvironmentHelper(),


remove whitspace changes

DeepDiver1975 · 2017-11-30T09:19:34Z

lib/private/User/AccountMapper.php

@@ -239,4 +239,4 @@ public function callForAllUsers($callback, $search, $onlySeen) {
 		$stmt->closeCursor();
 	}

-}
+}


remove whitespace changes

DeepDiver1975 · 2017-11-30T09:20:39Z

lib/private/MembershipManager.php

+	 *
+	 * @return bool
+	 */
+	public function addGroupUser($accountId, $backendGroupId) {


addGroupMemberSqlQuery throws UniqueConstraintViolationException - see PHPDoc
this method should do also throw it

DeepDiver1975 · 2017-11-30T09:21:16Z

lib/private/MembershipManager.php

+	 *
+	 * @return bool
+	 */
+	public function addGroupAdmin($accountId, $backendGroupId) {


same as with addGroupUser

DeepDiver1975 · 2017-11-30T09:22:12Z

lib/private/MembershipManager.php

+	 * return result for all groups.
+	 *
+	 * @param string|int|null $groupId
+	 * @param bool $isInternalGroupId


isInternalGroupId unknown
isBackendGroupId missing in PHPDoc

mrow4a · 2017-11-30T14:40:49Z

@DeepDiver1975 adjusted to comments

lock · 2019-08-01T20:01:50Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

mrow4a requested review from PVince81, tomneedham, butonic, DeepDiver1975 and patrickjahns October 5, 2017 20:38

mrow4a commented Oct 5, 2017

View reviewed changes

mrow4a added the Hacktoberfest label Oct 5, 2017

mrow4a self-assigned this Oct 5, 2017

mrow4a force-pushed the centr_group_org branch from d31f4d3 to 506df6b Compare October 5, 2017 20:52

mrow4a commented Oct 5, 2017

View reviewed changes

mrow4a force-pushed the centr_groups branch from 93ef106 to 240150d Compare October 5, 2017 21:06

mrow4a mentioned this pull request Oct 5, 2017

Central membership and group table #29107

Closed

24 tasks

mrow4a force-pushed the centr_group_org branch from 506df6b to 2ff6785 Compare October 5, 2017 21:13

mrow4a requested a review from jvillafanez October 9, 2017 13:09

mrow4a force-pushed the centr_groups branch 2 times, most recently from 0ad5420 to d694f08 Compare October 16, 2017 09:44

mrow4a force-pushed the centr_group_org branch 3 times, most recently from 71987fb to a63e8f3 Compare October 18, 2017 11:01

mrow4a mentioned this pull request Oct 18, 2017

[Central group table] Centr groups and memberships integration #29282

Merged

2 tasks

mrow4a force-pushed the centr_group_org branch from a63e8f3 to 2569541 Compare October 18, 2017 11:33

mrow4a force-pushed the centr_groups branch from daf4895 to 88dca13 Compare November 7, 2017 19:18

mrow4a force-pushed the centr_group_org branch 2 times, most recently from c5561a4 to 08ec5c9 Compare November 7, 2017 19:29

jvillafanez reviewed Nov 14, 2017

View reviewed changes

DeepDiver1975 requested changes Nov 24, 2017

View reviewed changes

mrow4a force-pushed the centr_groups branch 2 times, most recently from e64b67b to f90e106 Compare November 27, 2017 13:12

mrow4a force-pushed the centr_group_org branch 2 times, most recently from 3eb25b5 to b9b7342 Compare November 27, 2017 13:33

mrow4a force-pushed the centr_groups branch 2 times, most recently from 87cabc9 to fd86b44 Compare November 28, 2017 12:09

mrow4a force-pushed the centr_group_org branch 2 times, most recently from 4f3e235 to 0c46853 Compare November 28, 2017 13:02

DeepDiver1975 requested changes Nov 30, 2017

View reviewed changes

Add membership manager with unit tests

8c14e9c

mrow4a force-pushed the centr_group_org branch from 0c46853 to 8c14e9c Compare November 30, 2017 14:40

DeepDiver1975 added this to the development milestone Dec 1, 2017

DeepDiver1975 approved these changes Dec 1, 2017

View reviewed changes

DeepDiver1975 merged commit 5ad907c into centr_groups Dec 1, 2017

DeepDiver1975 deleted the centr_group_org branch December 1, 2017 11:39

ownclouders added the status/STALE label Dec 20, 2017

lock bot locked as resolved and limited conversation to collaborators Aug 1, 2019

		private $db;


		public function __construct(IDBConnection $db, IConfig $config) {

[Central group table] Add membership manager to central group table #29154

[Central group table] Add membership manager to central group table #29154

Conversation

mrow4a commented Oct 5, 2017 • edited Loading

mrow4a commented Oct 5, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrow4a Oct 5, 2017 • edited Loading

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 Oct 9, 2017

jvillafanez commented Oct 10, 2017

mrow4a commented Oct 11, 2017

mrow4a commented Nov 7, 2017

mrow4a commented Nov 14, 2017

Choose a reason for hiding this comment

DeepDiver1975 commented Nov 24, 2017

ownclouders commented Nov 24, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ownclouders commented Nov 24, 2017

mrow4a commented Nov 28, 2017

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

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 Nov 30, 2017

lock bot commented Aug 1, 2019

mrow4a commented Oct 5, 2017 •

edited

Loading

mrow4a Oct 5, 2017 •

edited

Loading