[PKP-LIB][main] #4860 Add JAV support to publications #10666
Conversation
There was a problem hiding this comment.
Looks good, @defstat! I've added some specific comments, but here are two general ones:
- I think we should remove the old
versioncolumn after migrating its data to the new columns. This will make sure we don't accidentally rely on it when it's completely supplanted by the new stuff. - I'd suggest we don't use the JAV acronym anywhere in this. I don't think it's a formally adopted acronym by the working group, and it's likely that folks will be generally familiar with it. Better to clarify all of our conversation around versions (e.g. removing the other column) so that we always mean a JAV version when we talk about a version, and not complicate the code with JAV where we specifically mean that standard/working group.
| @@ -111,7 +112,10 @@ class PKPSubmissionController extends PKPBaseController | |||
| 'getPublicationIdentifierForm', | |||
| 'getPublicationLicenseForm', | |||
| 'getPublicationTitleAbstractForm', | |||
| 'getChangeLanguageMetadata' | |||
| 'getChangeLanguageMetadata', | |||
| 'getJAVStageMetadata', | |||
There was a problem hiding this comment.
There's inconsistency between these two lines on JAV vs. Jav; I don't have a strong preference but they should be consistent. In any case, I think it would be better to avoid the acronym, as most won't be familiar with it. And listing stage and numbering in the API name is overspecific. Instead, maybe changeVersionData?
| 'getChangeLanguageMetadata', | ||
| 'getJAVStageMetadata', | ||
| 'changeJavStageAndNumbering', | ||
| 'getNextAvailableJavVersionNumberingForStage', |
There was a problem hiding this comment.
As above, maybe getNextAvailableVersionData?
| ], Response::HTTP_NOT_FOUND); | ||
| } | ||
|
|
||
| // $submission = Repo::submission()->get((int) $illuminateRequest->route('submissionId')); |
| <?php | ||
|
|
||
| /** | ||
| * @file publication/enums/JavStage.php |
There was a problem hiding this comment.
Because this refers to an external standard, it would be good to add a URL to where it's specified.
| { | ||
| $versionStageValue = $this->javStage->value; | ||
|
|
||
| return "$versionStageValue $this->javVersionMajor.$this->javVersionMinor"; |
There was a problem hiding this comment.
This should probably be localized.
|
|
||
| enum JavStage: string | ||
| { | ||
| case AUTHOR_ORIGINAL = 'AO'; |
There was a problem hiding this comment.
While these are fine to use in the codebase, we should be careful not to present them straight to the front end without having them map to translatable locale keys.
| public function getExistingJavStagesByPublications(): Collection | ||
| { | ||
| $publications = $this->getData('publications'); | ||
| if ($publications->isEmpty()) { |
There was a problem hiding this comment.
Aren't these 3 lines redundant? If the collection was empty, the result of this operation will be an empty collection anyway.
| msgid "publication.versionStage.minorOrMajor" | ||
| msgstr "Version Impact" | ||
|
|
||
| msgid "publication.versionStage.minorOrMajor.description" |
There was a problem hiding this comment.
This needs a translation, or to be removed if it's redundant. But I'd suggest not including the locale file changes -- they're likely to be made redundant by someone else's UI implementation.
No description provided.