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

Jump to bottom

pkp/pkp-lib#10090 Ensure anonymity in discussions for double blind reviews #10391

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

pkp/pkp-lib#10090 Ensure anonymity in discussions for double blind reviews #10391

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fec00a1
pkp/pkp-lib#10090 Ensure anonymity in discussions for double-blind re…
Hafsa-Naeem Sep 12, 2024
3fa4c3c
pkp/pkp-lib#10090 Avoid usage of column name outside of a query builder
Hafsa-Naeem Sep 23, 2024
b09c81b
pkp/pkp-lib#10090 fix conflicts and rebased
Hafsa-Naeem Oct 2, 2024
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
11 changes: 11 additions & 0 deletions classes/stageAssignment/StageAssignment.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,4 +145,15 @@ public function scopeWithContextId(Builder $query, ?int $contextId): Builder
->where('submissions.context_id', $contextId);
});
}


/**
* Accessor for the userId attribute.
*
* @return int The user_id as an integer.
*/
public function getUserIdAttribute(): int
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need this method in the PR? HasCamelCasing trait should handle all cases of transforming between camel and snake case when Model is instantiated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Vitaliy-1 Yes I also wanted to avoid adding the getUserIdAttribute accessor. But I had to implement this accessor because the pluck('userId') method does not trigger the HasCamelCasing transformation. This trait works for direct property access and method calls, but pluck directly queries the database fields, which are in snake-case (user_id). Without the accessor, pluck('userId') returns null values since it doesn't recognize the camel-case attribute, this is how the original code was and caused the issue of author names being visible in double-anonymous reviews.

I initially faced this issue when trying to use pluck('userId') directly, which is why I added the accessor to ensure the correct transformation.

If preferred, I can remove the accessor and revert to using the snake_case user_id like →pluck('user_id'), but this approach is not ideal within our codebase.

Another alternative could be using map() to iterate over the collection and access the userId attribute but it is less efficient than pluck() and introduces unnecessary overhead.

{
return (int) $this->attributes['user_id'];
}
}
11 changes: 4 additions & 7 deletions controllers/grid/queries/form/QueryForm.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,14 +327,11 @@ public function fetch($request, $template = null, $display = false, $actionArgs
// if current user is an anonymous reviewer, filter out authors
foreach ($reviewAssignments as $reviewAssignment) {
if ($reviewAssignment->getReviewerId() == $user->getId()) {
if ($reviewAssignment->getReviewMethod() != ReviewAssignment::SUBMISSION_REVIEW_METHOD_OPEN) {
// Replaces StageAssignmentDAO::getBySubmissionAndRoleId
$excludeUsers = StageAssignment::withSubmissionIds([$query->assocId])
if ($reviewAssignment->getReviewMethod() == ReviewAssignment::SUBMISSION_REVIEW_METHOD_DOUBLEANONYMOUS) {
$stageAssignments = StageAssignment::withSubmissionIds([$query->assocId])
->withRoleIds([Role::ROLE_ID_AUTHOR])
->withUserId($user->getId())
->get()
->pluck('userId')
->all();
->get();
$excludeUsers = $stageAssignments->pluck('userId')->all();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we retrieve excluded users without intermediate variables? Like:

$excludeUsers = StageAssignment::withSubmissionIds([$query->assocId])
    ->withRoleIds([Role::ROLE_ID_AUTHOR])
    ->get()
    ->pluck('userId')
    ->all();
    ```

Copy link
Contributor Author

@Hafsa-Naeem Hafsa-Naeem Oct 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'll update the code as you suggested here. But as mentioned in the comment about the accessor, I'll proceed with the final update once we decide on the approach regarding the accessor.

}
}
}
Expand Down