-
Notifications
You must be signed in to change notification settings - Fork 462
pkp/pkp-lib#10929 Temporary fix for Production Editor's file upload issues #11075
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.
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
Conversation
ff94a7b
to
2d81c3e
Compare
@@ -664,8 +664,34 @@ public function add(Request $illuminateRequest): JsonResponse | |||
: $submitAsUserGroup->permitMetadataEdit | |||
); | |||
|
|||
$isSubmittingAsProductionEditor = $submitAsUserGroup->getLocalizedData('name') === __('default.groups.name.productionEditor'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this is the best way to check for a specific role. Open to suggestions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the best way would be to check the access to the submission stage through the user group stage table
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot to mention that OPS might require different treatment - in 3.5 stage ID has only value 5
there, submission stage was removed.
3bf5692
to
38709e1
Compare
@Vitaliy-1 Thanks for the review. I've made the updates based on your suggestions. Also, given that OPS does not have Production Editor group, and production stage (stage ID |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the late reply!
The part of code that checks user roles and automatically assigns author role if necessary is here: https://github.com/pkp/pkp-lib/blob/main/pages/submission/PKPSubmissionHandler.php#L615
Thus, it makes senes to move the check there.
Moreover, I don't know what was the initial idea behind that part. In my opinion, it makes sense to check if user has access to the submission stage in general, and if not, assign an author role. That applies, for production editors, which are managers, copyeditors, layout editors, reviewers... and custom user groups.
Also, we might want to exclude user groups without access to submission stage from from the list here: https://github.com/pkp/pkp-lib/blob/main/classes/components/forms/submission/StartSubmission.php#L159
8538c6b
to
906217d
Compare
Thanks @Vitaliy-1, I've made updates based on your suggestions:
Take a look and let me know if it makes sense |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @taslangraham! I left 2 minor comments. I see that there were changes introduced in the stable branch that should be addressed during rebase and could alter this PR. Can you rebase and tag me for a quick look?
// There isn't a submission stage in OPS, so $groupsWithAccessToSubmissionStage will be empty there. | ||
// For other apps, filter $userGroups to be only those with access to submission stage | ||
if (!empty($groupsWithAccessToSubmissionStage)) { | ||
$userGroups = $userGroups->filter(function (UserGroup $userGroup) use ($groupsWithAccessToSubmissionStage) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could be replaced by shorted arrow function fn (UserGroup $userGroup) => ...
2d5c643
to
de7fcbc
Compare
Thanks for the review @Vitaliy-1! I've made the suggested changes and resolved all merge conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, just one small comment
// author role that allows self registration | ||
$userGroups = $isAdmin | ||
? $query->withRoleIds([Role::ROLE_ID_MANAGER, Role::ROLE_ID_SITE_ADMIN])->get() | ||
: $query->withStageIds([WORKFLOW_STAGE_ID_SUBMISSION])->get(); // For non-admin users, query for the groups tht give them access to the submission stage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, we also need to check start and end dates
8ad6cad
to
de7fcbc
Compare
…tage when creating a submission
…submit as' options"
379dd80
to
8cbe69a
Compare
8cbe69a
to
b60097d
Compare
For #10929