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

[Hafsa-Naeem]

for pkp/pkp-lib#10090

[CLAassistant]

All committers have signed the CLA.

[Vitaliy-1]

Thanks, it works perfectly! I've just added a small comment regarding our coding conventions. Also, I recommend using the issue number for the branch name, e.g., i10090_anonymity_discussions - just to not get lost in branches when working simultaneously on many issues.

[Vitaliy-1]

We avoid usage of column names outside of a query builder. I see that UseCamelCasing doesn't convert when not calling the model directly. If it's impossible to avoid, we can just create a scope for a Model. Here we can just get the collection first and then call pluck on the collection, how it was implemented before.

[Vitaliy-1]

@Hafsa-Naeem, is the PR ready? If yes, can you update it?

[Hafsa-Naeem]

@Vitaliy-1 Yes, I've made the necessary changes based on your feedback. The PR is now updated and ready for your review, let me know if anything else is needed.

[Vitaliy-1]

It still has conflicts with the QueryForm, can you rebase it locally on top of the main, and ping me to merge?
E.g. (depending on your setup):

git checkout main
git pull upstream main

depending on what main you are tracking, e.g., you can also update your own own fork's main in the Github interface

git checkout main
git pull origin main

You can always check what branch is tracked by:

git status
git remote -v

And then do rebase and deal with conflicts:

git checkout reviewer_anonimity_bug
git rebase -i main
git push --force 

[Hafsa-Naeem]

@Vitaliy-1 Thank you. I have resolved the conflicts and rebased the branch on top of the main. Please review.

[Vitaliy-1]

I don't see any changes comparing with the current main. Probably something got lost during rebase. Can you double check please?

[Hafsa-Naeem]

@Vitaliy-1 I’ve fixed the rebase issues and the changes. Please review. Thanks!

[Vitaliy-1]

Can we retrieve excluded users without intermediate variables? Like:

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

[Hafsa-Naeem]

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.

[Vitaliy-1]

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.

[Hafsa-Naeem]

@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.

[Vitaliy-1]

Thanks! I left comments, I think there is a method that was added to the StageAssignment by mistake