Added review_assignment_settings table #4580
Conversation
taslangraham
force-pushed
the
i10759-main
branch
2 times, most recently
from
ba50c6a to
c848334
Compare
taslangraham
force-pushed
the
i10759-main
branch
3 times, most recently
from
6f1a492 to
854f27a
Compare
| $reviewer = $review ? Repo::user()->get($review->getReviewerId()) : null; | ||
|
|
||
| if ($reviewer && $reviewer->getData('orcidReviewPutCode')) { | ||
| $context = app()->get('context')->get(Repo::submission()->get($review->getSubmissionId())->getData('contextId')); |
There was a problem hiding this comment.
I think for the time being it's probably best to stick with the Application::getContextDAO()->getById() way of getting a context for consistency.
|
|
||
| if ($reviewer && $reviewer->getData('orcidReviewPutCode')) { | ||
| $context = app()->get('context')->get(Repo::submission()->get($review->getSubmissionId())->getData('contextId')); | ||
| dispatch(new ReconcileOrcidReviewPutCode($reviewer->getId(), $context->getId())); |
There was a problem hiding this comment.
Having looked at it and thought about it more, I do think we want the DepositOrcidReview job to be dependent on the ReconcileOrcidReviewPutCode job. Your initial reasoning makes sense, but I think we do not want to try and deposit any reviews if the put codes have not properly been reconciled. The adverse consequences if the reconcile job fails and therefore the deposit job fails are the editor or someone will eventually need to retry it. if the reconcile job fails but the deposit job moves forward it could potentially overwrite data.
Obviously the reconcile job failing in either case isn't great, but I'd rather everything fail than potentially overwrite or add incorrect data to someone's ORCID record.
There was a problem hiding this comment.
Yea, this does make sense. Laravel's job-chaining can be used to accomplish this.
I'll update the code.
| if ($orcidAccessExpiresOn->isFuture()) { | ||
| OrcidManager::logInfo('Attempting to update put-codes for reviewer ' . $reviewer->getId()); | ||
|
|
||
| $context = app()->get('context')->get($this->contextId);/** @var Context $context */ |
There was a problem hiding this comment.
Same as comment above for method to fetch $context.
|
|
||
| // Remove old put-code from user object | ||
| $reviewer->setData('orcidReviewPutCode', null); | ||
| Repo::user()->edit($reviewer, ['orcidReviewPutCode']); |
There was a problem hiding this comment.
I did a cursory look, but it seems like this isn't actually deleting the orcidReviewPutCode from the user settings table. Could you have a look?
There was a problem hiding this comment.
I've noticed this issue elsewhere and have opened a issue with PR for it. Do you mind taking a look?
pkp/pkp-lib#10868
taslangraham
force-pushed
the
i10759-main
branch
2 times, most recently
from
c11b739 to
f2b326f
Compare
| @@ -160,6 +160,7 @@ | |||
| <migration class="APP\migration\upgrade\v3_5_0\I10620_EditorialBoardMemberRole"/> | |||
| <migration class="APP\migration\upgrade\v3_5_0\I9707_WeblateUILocales"/> | |||
| <note file="docs/release-notes/README-3.5.0" /> | |||
| <migration class="PKP\migration\upgrade\v3_5_0\I10759_AddReviewAssignmentSettings"/> | |||
There was a problem hiding this comment.
Sorry I didn't catch this before. This should go before the <note> tag, as that should be the last thing to run as part of the upgrade.
review_assignment table
taslangraham
force-pushed
the
i10759-main
branch
from
f2b326f to
bb97815
Compare
For pkp/pkp-lib#10759