pkp/pkp-lib#10506 Refactor UserGroup to use Eloquent #10519

Hafsa-Naeem · 2024-10-15T12:38:03Z

No description provided.

Vitaliy-1

Thanks! I've left a couple of comments but overall it looks good!

Vitaliy-1 · 2024-10-21T07:14:54Z

classes/userGroup/Repository.php

+     * Retrieve a keyed Collection (key = user_group_id, value = count) with the amount of active users for each UserGroup.
+     *
+     * @param int|null $contextId
+     * @return Collection
     */
    public function getUserCountByContextId(?int $contextId = null): Collection


Can we move this to the Model as a scope?

Vitaliy-1 · 2024-10-21T07:16:01Z

classes/userGroup/Repository.php

+     *
+     * @param array $params
+     * @return UserGroup
+     */
    public function newDataObject(array $params = []): UserGroup


I don't think that we need this method

Vitaliy-1 · 2024-10-21T07:20:44Z

classes/userGroup/Repository.php

+            self::forgetEditorialHistoryCache($userGroup->context_id);
+        }
+
+        $userGroup->delete();

        Hook::call('UserGroup::delete', [$userGroup]);


We can rely on the Model's events here. But this needs to be added to the list of the removed hooks, the same as for Announcements

Vitaliy-1 · 2024-10-21T10:37:59Z

classes/core/SettingsBuilder.php

@@ -110,6 +111,10 @@ public function insertGetId(array $values, $sequence = null)
        $settingValues->each(function (mixed $settingValue, string $settingName) use ($id, &$rows) {
            $settingName = Str::camel($settingName);
            if ($this->isMultilingual($settingName)) {
+                // if non-localized data passed, set the locale to default one
+                if (is_string($settingValue)) {


Not sure if it makes the code better. @jonasraoni, do you have an opinion whether we should allow to set multilingual value by the default locale?

Vitaliy-1 · 2024-10-21T10:39:10Z

classes/userGroup/Repository.php

+     * @param int $id
+     * @param int|null $contextId
+     * @return UserGroup|null
+     */
    public function get(int $id, ?int $contextId = null): ?UserGroup


I would move some of the logic from Repository to the Model. In particular, methods that are associated with basic operations with the entity, like this one

Vitaliy-1 · 2024-10-21T11:00:33Z

classes/userGroup/UserGroup.php

     */
-    public function getContextId(): ?int
+    public function setAttribute($key, $value)


I don't think we need to override setAttribute() here in order to allow to set multilingual values as usual strings. It's better to think about general solution

Vitaliy-1 · 2024-10-21T11:09:25Z

classes/userGroup/UserGroup.php

     */
-    public function setDefault($isDefault)
+    public function stageAssignments(): HasMany


Can you also add the relationship to the StageAssignment? And the same for the UserUserGroup

Vitaliy-1 · 2024-10-21T11:24:11Z

classes/userGroup/Repository.php

-                return collect($item)->unique();
-            });
-            return $userIdsByUserGroupId->toArray();
+            $users = UserUserGroup::query()


Would it work without query() method applied?

Vitaliy-1 · 2024-10-21T11:29:43Z

classes/userGroup/Repository.php

+        $cacheKeyPrefix = 'PKP\userGroup\Repository::getMastheadUserIdsByRoleIds';
+        $cacheKeys = [
+            "{$cacheKeyPrefix}EditorialMasthead{$contextId}" . self::MAX_EDITORIAL_MASTHEAD_CACHE_LIFETIME,
+            "{$cacheKeyPrefix}EditorialHistory{$contextId}" . self::MAX_EDITORIAL_MASTHEAD_CACHE_LIFETIME,


Should this method also clear cache for the Editorial History?

Vitaliy-1 · 2024-10-21T11:33:00Z

classes/userGroup/Repository.php

@@ -34,6 +34,7 @@
 use PKP\userGroup\relationships\UserUserGroup;
 use PKP\validation\ValidatorFactory;
 use PKP\xml\PKPXMLParser;
+use Illuminate\Database\Query\Builder;


Several use statements aren't used anymore.

Vitaliy-1

Thanks, that's a huge piece of work!
I left comments, in general they are all small. I recommend to use scopes as per pattern in other refactored Models

Vitaliy-1 · 2024-10-30T15:53:54Z

api/v1/submissions/PKPSubmissionController.php

-            ->getMany();
+        $submitterUserGroups = UserGroup::where('contextId', $context->getId())
+            ->where('roleId', Role::ROLE_ID_MANAGER)
+            ->orWhere('roleId', Role::ROLE_ID_AUTHOR)


We have several where clauses here, do we need to group this orWhere as per eloquent documentation?
Or we could use whereIn

Vitaliy-1 · 2024-10-30T15:54:44Z

api/v1/submissions/PKPSubmissionController.php


        $userGroupIdPropName = 'userGroupId';

        if (isset($params[$userGroupIdPropName])) {
            $submitAsUserGroup = $submitterUserGroups
                ->first(function (UserGroup $userGroup) use ($params, $userGroupIdPropName) {
-                    return $userGroup->getId() === $params[$userGroupIdPropName];
+                    return $userGroup->usergroupid === $params[$userGroupIdPropName];


I believe here and in other places it could be replaced with $userGroup->id

Vitaliy-1 · 2024-10-30T15:59:21Z

api/v1/submissions/PKPSubmissionController.php

@@ -624,14 +621,14 @@ public function add(Request $illuminateRequest): JsonResponse
                })
                ->first();
        } else {
-            $submitAsUserGroup = Repo::userGroup()->getFirstSubmitAsAuthorUserGroup($context->getId());
+            $submitAsUserGroup = UserGroup::where('contextId', $context->getId())->where('roleId', Role::ROLE_ID_AUTHOR)->first();


According to the getFirstSubmit... method's body, it uses also permitSelfRegistration filter. I see you already have a scope for it to apply here

Vitaliy-1 · 2024-10-30T16:02:31Z

api/v1/submissions/PKPSubmissionController.php

-            ->filterByContextIds([$submission->getData('contextId')])
-            ->getMany();
+        $contextId = $submission->getData('contextId');
+        $userGroups = UserGroup::where('context_id', $contextId)->get();


UserGroup::where('contextId', $contextId)->get()

Vitaliy-1 · 2024-10-30T16:03:36Z

api/v1/submissions/PKPSubmissionController.php

-            ->getCollector()
-            ->filterByContextIds([$submission->getData('contextId')])
-            ->getMany();
+        $contextId = $submission->getData('contextId');


Here and in other places, it could be just one line:

$userGroups = UserGroup::where('contextId', $submission->getData('contextId'))->get()

Vitaliy-1 · 2024-11-05T19:55:46Z

classes/userGroup/Repository.php

+
+        // Determine masthead status
+        $masthead = match ($mastheadStatus) {
+            UserUserGroupMastheadStatus::STATUS_ON => 1,


masthead should be true/false

Vitaliy-1 · 2024-11-05T19:58:18Z

classes/userGroup/Repository.php

+                'permit_metadata_edit' => $permitMetadataEdit,
+                'is_default' => true,
+                'show_title' => true,
+                'masthead' => $masthead,


Can you double check if we are settings boolean true/false values and not a string?

Vitaliy-1 · 2024-11-05T19:58:33Z

classes/userGroup/Repository.php


-            // insert the group into the DB
-            $userGroupId = $this->add($userGroup);
+            $userGroupId = $userGroup->user_group_id;


Vitaliy-1 · 2024-11-05T19:59:26Z

classes/userGroup/Repository.php

-            $userGroup->setMasthead($masthead ?? false);
+            // Create a new UserGroup instance and set attributes
+            $userGroup = new UserGroup([
+                'role_id' => $roleId,


Here and in other places, can we use camel case when creating a model with attributes?

Vitaliy-1 · 2024-11-05T20:02:49Z

classes/userGroup/Repository.php

+            $abbrevKey = $userGroup->settings['abbrevLocaleKey'] ?? null;
+
+            if ($nameKey) {
+                $userGroup->settings['name'] = __($nameKey, [], $locale);


It should be possible to interact with and set settings attributes normally

…ing and Eloquent conventions

pkp#10506 Refactor UserGroup to use Eloquent

35b9299

Hafsa-Naeem requested a review from Vitaliy-1 October 15, 2024 12:38

Hafsa-Naeem mentioned this pull request Oct 15, 2024

Port UserGroup to Use Eloquent Model #10506

Open

Vitaliy-1 requested changes Oct 21, 2024

View reviewed changes

pkp#10506 Update usage for porting UserGroup to Eloquent

f20097d

Vitaliy-1 requested changes Nov 5, 2024

View reviewed changes

Hafsa-Naeem added 3 commits November 15, 2024 13:52

pkp#10506 Fix UserGroup and Repository code to correct settings handl…

1ce4b3f

…ing and Eloquent conventions

pkp#10506 optimize relationships with eager loading

2ef27fa

lib/pkp-lib#10506 Fix type errors and refactor with scopes

08c53ec

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkp/pkp-lib#10506 Refactor UserGroup to use Eloquent #10519

pkp/pkp-lib#10506 Refactor UserGroup to use Eloquent #10519

Hafsa-Naeem commented Oct 15, 2024

Vitaliy-1 left a comment

Vitaliy-1 Oct 21, 2024

Vitaliy-1 Oct 21, 2024

Vitaliy-1 Oct 21, 2024

Vitaliy-1 Oct 21, 2024

Vitaliy-1 Oct 21, 2024

Vitaliy-1 Oct 21, 2024

Vitaliy-1 Oct 21, 2024

Vitaliy-1 Oct 21, 2024

Vitaliy-1 Oct 21, 2024

Vitaliy-1 Oct 21, 2024

Vitaliy-1 left a comment

Vitaliy-1 Oct 30, 2024

Vitaliy-1 Oct 30, 2024

Vitaliy-1 Oct 30, 2024

Vitaliy-1 Oct 30, 2024

Vitaliy-1 Oct 30, 2024

Vitaliy-1 Nov 5, 2024

Vitaliy-1 Nov 5, 2024

Vitaliy-1 Nov 5, 2024

Vitaliy-1 Nov 5, 2024

Vitaliy-1 Nov 5, 2024

pkp/pkp-lib#10506 Refactor UserGroup to use Eloquent #10519

Are you sure you want to change the base?

pkp/pkp-lib#10506 Refactor UserGroup to use Eloquent #10519

Conversation

Hafsa-Naeem commented Oct 15, 2024

Vitaliy-1 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vitaliy-1 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment