Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pkp/pkp-lib#10751 EditorialMasthead page issue with eloquent based user group update #10754

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+72 −42
Open

pkp/pkp-lib#10751 EditorialMasthead page issue with eloquent based user group update #10754

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2f68f8f
pkp/pkp-lib#10751 EditorialMasthead page issue with eloquent based us…
touhidurabir Dec 27, 2024
a236303
pkp/pkp-lib#10751 fixed order by query
touhidurabir Dec 28, 2024
9fd3940
pkp/pkp-lib#10751 update of editorial history page considering user g…
touhidurabir Jan 3, 2025
a93422e
pkp/pkp-lib#10751 fixed missing UserUserGroup::withUserGroupIds int t…
touhidurabir Jan 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion classes/invitation/invitations/userRoleAssignment/rules/AddUserGroupRule.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ public function passes($attribute, $value)
// At this point, we know the user group exists; check if the user has it assigned
if ($user = $this->invitation->getExistingUser()) {
$userUserGroups = UserUserGroup::withUserId($user->getId())
->withUserGroupId($value) // The $value is the userGroupId
->withUserGroupIds([$value]) // The $value is the userGroupId
->withActive()
->get();

Expand Down
2 changes: 1 addition & 1 deletion classes/mail/mailables/UserRoleAssignmentInvitationNotify.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ public function setData(?string $locale = null): void

foreach ($userGroups as $userGroup) {
$userUserGroups = UserUserGroup::withUserId($user->getId())
->withUserGroupId($userGroup->id)
->withUserGroupIds([$userGroup->id])
->withActive()
->get();

Expand Down
2 changes: 1 addition & 1 deletion classes/services/PKPContextService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -562,7 +562,7 @@ public function add($context, $request)

$assignmentExists = UserUserGroup::query()
->withUserId($currentUser->getId())
->withUserGroupId($managerUserGroup->id)
->withUserGroupIds([$managerUserGroup->id])
->exists();

if (!$assignmentExists) {
Expand Down
4 changes: 2 additions & 2 deletions classes/user/Repository.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,7 +344,7 @@ public function mergeUsers(int $oldUserId, int $newUserId)
$submissionCommentDao = DAORegistry::getDAO('SubmissionCommentDAO'); /** @var SubmissionCommentDAO $submissionCommentDao */
$submissionComments = $submissionCommentDao->getByUserId($oldUserId);

while ($submissionComment = $submissionComments->next()) {
while ($submissionComment = $submissionComments->next()) { /** @var \PKP\submission\SubmissionComment $submissionComment */
$submissionComment->setAuthorId($newUserId);
$submissionCommentDao->updateObject($submissionComment);
}
Expand All @@ -369,7 +369,7 @@ public function mergeUsers(int $oldUserId, int $newUserId)
// Check if the new user is already assigned to this user group
$exists = UserUserGroup::query()
->withUserId($newUserId)
->withUserGroupId($userUserGroup->userGroupId)
->withUserGroupIds([$userUserGroup->userGroupId])
->exists();

if (!$exists) {
Expand Down
2 changes: 1 addition & 1 deletion classes/user/maps/Schema.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ protected function mapByProperties(array $props, User $user, array $auxiliaryDat
'recommendOnly' => (bool) $userGroup->recommendOnly,
'dateStart' => UserUserGroup::withUserId($user->getId())
->withActive()
->withUserGroupId($userGroup->id)
->withUserGroupIds([$userGroup->id])
->pluck('date_start')->first()
];
}
Expand Down
66 changes: 47 additions & 19 deletions classes/userGroup/Repository.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,23 +13,25 @@

namespace PKP\userGroup;

use Carbon\Carbon;
use DateInterval;
use Illuminate\Support\Collection;
use Illuminate\Support\Facades\Cache;
use Illuminate\Support\LazyCollection;
use Carbon\Carbon;
use PKP\core\Core;
use PKP\db\DAORegistry;
use PKP\plugins\Hook;
use PKP\site\SiteDAO;
use PKP\security\Role;
use PKP\db\DAORegistry;
use PKP\facades\Locale;
use PKP\xml\PKPXMLParser;
use Illuminate\Support\Collection;
use PKP\services\PKPSchemaService;
use PKP\site\SiteDAO;
use PKP\userGroup\relationships\enums\UserUserGroupMastheadStatus;
use PKP\userGroup\relationships\enums\UserUserGroupStatus;
use PKP\userGroup\relationships\UserGroupStage;
use PKP\userGroup\relationships\UserUserGroup;
use PKP\validation\ValidatorFactory;
use PKP\xml\PKPXMLParser;
use Illuminate\Support\Facades\Cache;
use Illuminate\Support\LazyCollection;
use Illuminate\Database\Query\JoinClause;
use PKP\userGroup\relationships\UserUserGroup;
use PKP\userGroup\relationships\UserGroupStage;
use PKP\userGroup\relationships\enums\UserUserGroupStatus;
use PKP\userGroup\relationships\enums\UserUserGroupMastheadStatus;

class Repository
{
Expand Down Expand Up @@ -218,7 +220,7 @@ public function userInGroup(int $userId, int $userGroupId): bool
{
return UserUserGroup::query()
->withUserId($userId)
->withUserGroupId($userGroupId)
->withUserGroupIds([$userGroupId])
->withActive()
->exists();
}
Expand All @@ -242,7 +244,7 @@ public function userOnMasthead(int $userId, ?int $userGroupId = null): bool
->withMasthead();

if ($userGroupId) {
$query->withUserGroupId($userGroupId);
$query->withUserGroupIds([$userGroupId]);
}

return $query->exists();
Expand All @@ -256,7 +258,7 @@ public function getUserUserGroupMastheadStatus(int $userId, int $userGroupId): U
{
$masthead = UserUserGroup::query()
->withUserId($userId)
->withUserGroupId($userGroupId)
->withUserGroupIds([$userGroupId])
->withActive()
->pluck('masthead')
->first();
Expand Down Expand Up @@ -344,7 +346,7 @@ public function deleteAssignmentsByUserId(int $userId, ?int $userGroupId = null)
$query = UserUserGroup::query()->withUserId($userId);

if ($userGroupId) {
$query->withUserGroupId($userGroupId);
$query->withUserGroupIds([$userGroupId]);
$userGroup = $this->get($userGroupId);
if ($userGroup && $userGroup->masthead) {
self::forgetEditorialCache($userGroup->contextId);
Expand Down Expand Up @@ -374,7 +376,7 @@ public function endAssignments(int $contextId, int $userId, ?int $userGroupId =
->withActive();

if ($userGroupId) {
$query->withUserGroupId($userGroupId);
$query->withUserGroupIds([$userGroupId]);
}

$query->update(['date_end' => $dateEnd]);
Expand Down Expand Up @@ -590,12 +592,38 @@ public function getMastheadUserIdsByRoleIds(array $mastheadRoles, int $contextId
$users = UserUserGroup::query()
->withContextId($contextId)
->withUserGroupIds($mastheadRoleIds)
->withUserUserGroupStatus($userUserGroupStatus->value)
->withUserUserGroupMastheadStatus(UserUserGroupMastheadStatus::STATUS_ON->value)
->whereHas(
'userGroup',
fn (\Illuminate\Database\Eloquent\Builder $query) => $query->withUserUserGroupStatus($userUserGroupStatus->value)
)
->withMasthead()
->orderBy('user_groups.role_id', 'asc')
->join('user_groups', 'user_user_groups.user_group_id', '=', 'user_groups.user_group_id')
->join('users', 'user_user_groups.user_id', '=', 'users.user_id')
->orderBy('users.family_name', 'asc')
->orderBy(
fn (\Illuminate\Database\Query\Builder $query) => $query
->fromSub(
fn($query) => $query->from(null)->selectRaw(0),
'placeholder'
)
->leftJoin(
'user_settings AS l',
fn (JoinClause $join) => $join
->on('user_user_groups.user_id', '=', 'l.user_id')
->where('l.setting_name', 'family_name')
->where('l.locale', Locale::getLocale())
)
->leftJoin(
'user_settings AS p',
fn (JoinClause $join) => $join
->on('user_user_groups.user_id', '=', 'p.user_id')
->where('p.setting_name', 'family_name')
->where('p.locale', Locale::getPrimaryLocale())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Locale::getPrimaryLocale() would return context primary locale if possible, right? The user multilingual data do not necessarily exist in the context primary locale -- the fall back locale for required users data is the site primary locale.
Also, I think we need to consider the given_name as well i.e. the logic should be the same as in the user Collector class, because it can be that there is no family name, so we should in that case consider/order by given name (that is required and thus always there at least in the site primary locale).
I hope I understand the code correctly... :-)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm... here https://github.com/pkp/pkp-lib/pull/10667/files#diff-c8cb812ae82cc024166167807c2d42818fe8c33bc61dc597835f7dbf2d7c91b2, the user collector did not change, so maybe to use the old part of the code here, that was changed/replaced in this PR, i.e. to user the user collector to get the users? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Locale::getPrimaryLocale() would return context primary locale if possible, right? The user multilingual data do not necessarily exist in the context primary locale -- the fall back locale for required users data is the site primary locale

Make sense . I think in that case it should be Application::get()->getRequest()->getSite()->getPrimaryLocale() .

I think we need to consider the given_name as well i.e. the logic should be the same as in the user Collector class, because it can be that there is no family name, so we should in that case consider/order by given name

@bozana If I understand the previous code correctly , it only used to consider the family_name as the previous code was like

// Sort the results by role ID and user family name.
$usersCollector = Repo::user()->getCollector();
$usersQuery = $usersCollector
    ->filterByContextIds([$contextId])
    ->filterByUserGroupIds($mastheadRolesIds)
    ->filterByUserUserGroupStatus($userUserGroupStatus)
    ->filterByUserUserGroupMastheadStatus(UserUserGroupMastheadStatus::STATUS_ON)
    ->orderBy($usersCollector::ORDERBY_FAMILYNAME, $usersCollector::ORDER_DIR_ASC, [Locale::getLocale(), Application::get()->getRequest()->getSite()->getPrimaryLocale()])
    ->orderByUserGroupIds($mastheadRolesIds)
    ->getQueryBuilder()
    ->get();

so if we want to also order by given_name that will be double ordering , not seems like a problem if I understand correctly .

the user collector did not change, so maybe to use the old part of the code here, that was changed/replaced in this PR

Thats also possible and in this case perhaps better . Otherwise we will need to maintain similar implementation in multiple places .

Copy link
Collaborator

@bozana bozana Jan 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I understand it correctly, that here both family but then also given name will be considered here: https://github.com/pkp/pkp-lib/blob/main/classes/user/Collector.php#L779-L814 -- which was used earlier... ?
It goes through foreach ($sortedSettings as $i => $setting) and uses then COALESCE
I think it would be easier to use it how it was -- no need to code it double then...

)
->selectRaw(
'CONCAT(COALESCE(l.setting_value, ""), COALESCE(p.setting_value, ""))'
)
)
->get(['user_groups.user_group_id', 'users.user_id']);

// group unique user ids by UserGroup id
Expand Down
10 changes: 5 additions & 5 deletions classes/userGroup/relationships/UserUserGroup.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,13 @@

namespace PKP\userGroup\relationships;

use PKP\core\Core;
use APP\facades\Repo;
use PKP\userGroup\UserGroup;
use Eloquence\Behaviours\HasCamelCasing;
use Illuminate\Database\Eloquent\Builder;
use Illuminate\Database\Eloquent\Casts\Attribute;
use Illuminate\Database\Eloquent\Relations\BelongsTo;
use Eloquence\Behaviours\HasCamelCasing;
use PKP\core\Core;
use PKP\userGroup\UserGroup;

class UserUserGroup extends \Illuminate\Database\Eloquent\Model
{
Expand Down Expand Up @@ -52,9 +52,9 @@ public function scopeWithUserId(Builder $query, int $userId): Builder
return $query->where('user_user_groups.user_id', $userId);
}

public function scopeWithUserGroupId(Builder $query, int $userGroupId): Builder
public function scopeWithUserGroupIds(Builder $query, array $userGroupIds): Builder
{
return $query->where('user_user_groups.user_group_id', $userGroupId);
return $query->whereIn('user_user_groups.user_group_id', $userGroupIds);
}

public function scopeWithActive(Builder $query): Builder
Expand Down
2 changes: 1 addition & 1 deletion controllers/grid/settings/user/form/UserForm.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ public function saveUserGroupAssignments(Request $request): void
fn ($userGroupId) =>
UserUserGroup::query()
->withUserId($this->userId)
->withUserGroupId($userGroupId)
->withUserGroupIds([$userGroupId])
->withActive()
->update(['date_end' => now()])
);
Expand Down
12 changes: 7 additions & 5 deletions pages/about/AboutContextHandler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
use APP\handler\Handler;
use APP\template\TemplateManager;
use DateTime;
use Illuminate\Support\Collection;
use PKP\context\Context;
use PKP\facades\Locale;
use PKP\orcid\OrcidManager;
use PKP\plugins\Hook;
Expand Down Expand Up @@ -61,7 +63,7 @@ public function index($args, $request)
}


private function getSortedMastheadUserGroups($context)
private function getSortedMastheadUserGroups(Context $context): Collection
{
$mastheadUserGroups = UserGroup::withContextIds([$context->getId()])
->masthead(true)
Expand Down Expand Up @@ -95,7 +97,7 @@ public function editorialMasthead($args, $request)

// Get all user IDs grouped by user group ID for the masthead roles
$allUsersIdsGroupedByUserGroupId = Repo::userGroup()->getMastheadUserIdsByRoleIds(
$mastheadRoles,
$mastheadRoles->all(),
$context->getId()
);

Expand All @@ -104,7 +106,7 @@ public function editorialMasthead($args, $request)
foreach ($allUsersIdsGroupedByUserGroupId[$userGroupId] ?? [] as $userId) {
$user = Repo::user()->get($userId);
$userUserGroup = UserUserGroup::withUserId($user->getId())
->withUserGroupId($userGroupId)
->withUserGroupIds([$userGroupId])
->withActive()
->withMasthead()
->first();
Expand Down Expand Up @@ -161,7 +163,7 @@ public function editorialHistory($args, $request)

// get all user IDs grouped by user group ID for the masthead roles with ended status
$allUsersIdsGroupedByUserGroupId = Repo::userGroup()->getMastheadUserIdsByRoleIds(
$mastheadRoles,
$mastheadRoles->all(),
$context->getId(),
UserUserGroupStatus::STATUS_ENDED
);
Expand All @@ -171,7 +173,7 @@ public function editorialHistory($args, $request)
foreach ($allUsersIdsGroupedByUserGroupId[$userGroupId] ?? [] as $userId) {
$user = Repo::user()->get($userId);
$userUserGroups = UserUserGroup::withUserId($user->getId())
->withUserGroupId($userGroupId)
->withUserGroupIds([$userGroupId])
->withEnded()
->withMasthead()
->orderBy('date_start', 'desc')
Expand Down
6 changes: 3 additions & 3 deletions templates/frontend/pages/editorialHistory.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,10 @@
<h1>{translate key="common.editorialHistory.page"}</h1>
<p>{translate key="common.editorialHistory.page.description"}</p>
{foreach from=$mastheadRoles item="mastheadRole"}
{if array_key_exists($mastheadRole->getId(), $mastheadUsers)}
<h2>{$mastheadRole->getLocalizedName()|escape}</h2>
{if array_key_exists($mastheadRole->id, $mastheadUsers)}
<h2>{$mastheadRole->getLocalizedData('name')|escape}</h2>
<ul class="user_listing" role="list">
{foreach from=$mastheadUsers[$mastheadRole->getId()] item="mastheadUser"}
{foreach from=$mastheadUsers[$mastheadRole->id] item="mastheadUser"}
<li>
{strip}
<span class="date_start">
Expand Down
6 changes: 3 additions & 3 deletions templates/frontend/pages/editorialMasthead.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,10 @@

<h1>{translate key="common.editorialMasthead"}</h1>
{foreach from=$mastheadRoles item="mastheadRole"}
{if array_key_exists($mastheadRole->getId(), $mastheadUsers)}
<h2>{$mastheadRole->getLocalizedName()|escape}</h2>
{if array_key_exists($mastheadRole->id, $mastheadUsers)}
<h2>{$mastheadRole->getLocalizedData('name')|escape}</h2>
<ul class="user_listing" role="list">
{foreach from=$mastheadUsers[$mastheadRole->getId()] item="mastheadUser"}
{foreach from=$mastheadUsers[$mastheadRole->id] item="mastheadUser"}
<li>
{strip}
<span class="date_start">{translate key="common.fromUntil" from=$mastheadUser['dateStart'] until=""}</span>
Expand Down