PKP Bugzilla – Bug 6406
OMP clean-up: Clean up review information in submissions
Last modified: 2013-11-27 16:51:05 PST
We are moving to Git Issues for bug tracking in future releases. During transition, content will be in both tools. If you'd like to file a new bug, please create an issue.
See Monograph::get/setCurrentRound(), get/setReviewRoundsInfo():
- undocumented methods
- current review round is redundant with information in review rounds entity (potential for inconsistency, not 3rd NF)
- review round info is an unstructured array (not OO, should probably be ReviewRound objects retrieved from ReviewRoundDao, see design options for DAOs in the Wiki: http://pkp.sfu.ca/wiki/index.php?title=Data_Access_Objects_(DAO))
Do we need the ReviewerSubmission? It's used in very few places in OMP and it seems that we could as well use a normal Monograph implementation instead.
*** Bug 5911 has been marked as a duplicate of this bug. ***
The overall design of the xxxSubmission classes (e.g. ReviewerSubmission, AuthorSubmission, etc.) predates me, but they were originally planned as a partial solution to authorization, i.e. Reviewers should broadly be able to alter these objects, but looking at the ReviewerSubmissionDAO::updateReviewerSubmission function in OJS, only certain fields are updated. This is somewhat more apparent in OJS in SectionEditorSubmissionDAO::updateSectionEditorSubmission -- look for the comment "Only update fields that can actually be edited" for an example.
This has always been pretty clumsy IMO.
Aside from this, the xxxSubmission* classes are used to extend the base class in a couple of useful ways:
1) Fetching the appropriate files for the role in xxxSubmission; I can't think of any reason this couldn't be done using a lazy-load approach, and probably doesn't apply to OMP anyway because we aren't talking about a single-file-per-submission model
2) Providing a few useful methods in xxxSubmissionDAO that don't make sense elsewhere, like fetching counts for reviewer submissions, and getting submission lists in the various role-specific queues
In OMP's case, I can't think of any reason OTOH why the xxxSubmission classes are necessary, though I haven't done a thorough grep/review. The xxxSubmissionDAO classes might still prove useful, however.
I didn't want to do away with all xxxSubmission classes in this bug report. Very specifically only the ReviewerSubmission. I see the xxxSubmission topic more in the context of cleaning up DAOs and the corresponding domain objects (see migration issues). IMO a lot of the confusion arises because we mix up use cases (aka roles) which should be handled by (transaction oriented) controllers and (static) domain objects.
change reviewRoundInfo to return iterator of ReviewRound objects