Bug 6406

Summary: OMP clean-up: Clean up review information in submissions
Product: OMP Reporter: jerico <jerico.dev>
Component: GeneralAssignee: PKP Support <pkp-support>
Status: NEW ---    
Severity: normal CC: alec
Priority: P3    
Version: To Be Determined   
Hardware: PC   
OS: Linux   
Version Reported In: Also Affects:
Bug Depends on:    
Bug Blocks: 6410    

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