-
Notifications
You must be signed in to change notification settings - Fork 470
pkp/pkp-lib#12035 (3.4.0) Fix issue where recommend-only users cannot add a new review round #12037
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
base: stable-3_4_0
Are you sure you want to change the base?
Conversation
| } elseif ($stageAssignment->getRecommendOnly()) { | ||
| if ($decisionType->getDecision() === Decision::NEW_EXTERNAL_ROUND) { | ||
| $isAllowed = true; | ||
| } |
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 all these ifs can be simplified into:
| } elseif ($stageAssignment->getRecommendOnly()) { | |
| if ($decisionType->getDecision() === Decision::NEW_EXTERNAL_ROUND) { | |
| $isAllowed = true; | |
| } | |
| $decision = $decisionType->getDecision(); | |
| if (!$stageAssignment->getRecommendOnly() || | |
| $decision === Decision::NEW_EXTERNAL_ROUND || | |
| Repo::decision()->isRecommendation($decision)) { | |
| $isAllowed = true; | |
| break; | |
| } |
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.
In fact, after checking the other nearby lines, I think a lot of improvements should be added here:
- The only purpose of the
foreachloop seems to perhaps set the$isAllowedto true, therefore, abreakcan be added once the value is set to true... - On the lines 62 and 66: the
elseisn't needed and can be removed, a decreased indentation simplifies reading code. - Looks like the block ranging from line 84 until 96 has no dependency with the inner variables of the
foreachloop, therefore, it doesn't need to be called many times (there's a query involved and this is expensive) and should be cached... (e.g.$cached ??= (function (){...})();or using an extra private method)
asmecher
left a comment
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'm ambivalent about this change -- if you were asking me about whether recommend-only users should be able to start a new round, I'd probably say no (not that I'm particularly informed about this aspect of workflow); however, this is a regression from 3.3.0, probably an accidental one made during refactor of editorial decisions. So I'm OK either way.
However, we probably should stay consistent in this decision between internal and external reviews (for OMP).
jonasraoni
left a comment
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've never touched this piece of code, but I've left a couple of suggestions, which I think are good to handle while there's someone touching this.
|
@asmecher Since this is working fine in 3.4, could you please apply it to 3.4 for now and consider further improvements in later versions? I’ve just simplified the if statements as @jonasraoni suggested, while the remaining enhancements can be addressed for 3.5 and beyond. |
ead1ecf to
5d52581
Compare
|
@salmanm2003, I'll leave that decision to @Vitaliy-1 as the workflow lead. |
This is for: #12035