Bug 6406 - OMP clean-up: Clean up review information in submissions
OMP clean-up: Clean up review information in submissions
Status: NEW
Product: OMP
Classification: Unclassified
Component: General
To Be Determined
PC Linux
: P3 normal
Assigned To: PKP Support
: 5911 (view as bug list)
Depends on:
Blocks: 6410
  Show dependency treegraph
 
Reported: 2011-02-14 09:03 PST by jerico
Modified: 2013-11-27 16:51 PST (History)
1 user (show)

See Also:
Version Reported In:
Also Affects:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description jerico 2011-02-14 09:03:35 PST
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))
Comment 1 jerico 2011-02-14 15:03:25 PST
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.
Comment 2 jerico 2011-02-14 15:32:45 PST
*** Bug 5911 has been marked as a duplicate of this bug. ***
Comment 3 Alec Smecher 2011-02-14 15:37:10 PST
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.
Comment 4 jerico 2011-02-14 15:54:38 PST
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.
Comment 5 Juan Pablo Alperin 2011-07-22 15:40:02 PDT
change reviewRoundInfo to return iterator of ReviewRound objects
https://github.com/pkp/omp/commit/eebc0dd76ae1aa14637a449d803ddb468d6f963e