[Submission Workflow] | Internal Review stage - Editorial revision upload flow, upload button uploads file to the files for review table

[Tribunal33]

Valid Title

• I have updated the title to accurately reflect the bug description

Description

This is part of an ongoing investigation into #11219 as an attempt to see if I can figure out the root cause of the issue. There is a similar issue here #7144. My current hypothesis is that this button is simply adding the file to the wrong table or the author should not be able to view the Files for Review list when uploading a revision. If I click on the upload file in the revision table I expect the file to be uploaded to this table. There are several problems when an author is requested to do revisions with regards to this issue. This occurs on OMP 3.4 as well as 3.5 the buttons and table names are slightly different.

Preconditions: Should have a submission in review that has completed the submission round with files that are expected to be reviewed.

Steps to Reproduce

As an Editor:

1. Login with Editor role that can login as author
2. Navigate to the submission in the internal review round
3. Observe the files in the Files for Review table. In the next step we will revise one of them
4. Click on Upload from the Revisions Uploaded table
5. From the drop down select one of the files from the list to revise
6. Upload a revised file and change the name so it is easy to spot
7. Complete the file upload flow
8. Observe the Review Files table

As an Author:

1. Follow the same steps as editor except you cannot observe the Review Files table
2. Upload a revision from an existing file from the drop down list
3. Complete the upload and observe that nothing is displayed for the author.

Expected Result

As an Editor I believe if you upload a file to the revisions table it should be placed in the revisions table. Or These files should not be part of the list of files to revise. If you wanted to replace a file then you should do that in the upload files in the Files for Review table.

As an author. I believe you should see the file that was revised in the Revisions table. Or these files should be removed from the list of potential files to be revised.

Actual Result

The file that is revised will be replaced with the newly revised file in the Review files table. Only if you select "This is not a revision of an existing file" will the newly added file go into the Revisions table.

For the author even though they upload a revision of an existing file it is hidden from them because authors can not see the Review files table.

Environment Details

No response

Application Version

OMP 3.5.0rc2

Logs

No response

Additional Information

No response

[jardakotesovec]

Even though this bug report is focused on internal review. Lets first establish how it should work for external review round in OJS. Behaviour in internal is probably expected to be the same.

I did test 3.3, 3.4 and 3.5.
The latest logic for this was introduced in #3653 . Which is even before 3.3.

From the code below its clear that there is intention to exclude submission files for Authors, but show them for other roles. But there is mistake which cause not to show these files to neither authors nor editors. This was accidentally fixed in 3.5.

And question is whether to stick with the behaviour of 3.3 or bring the behaviour which was probably originally intended.

Logic in 3.3

$stageAssignmentDao->getBySubmissionAndRoleId is always true, because it returns some DAOResultFactory, not actual result. Therefore option to select from revision list never shows up.

		foreach ($allSubmissionFiles as $submissionFile) {
			// The uploaded file must be excluded from the list of revisable files.
			if ($uploadedFile && $uploadedFile->getId() == $submissionFile->getId()) continue;
			if (
				($submissionFile->getFileStage() == SUBMISSION_FILE_REVIEW_ATTACHMENT || $submissionFile->getFileStage() == SUBMISSION_FILE_REVIEW_FILE) &&
				$stageAssignmentDao->getBySubmissionAndRoleId($submissionFile->getData('submissionId'), ROLE_ID_AUTHOR, $this->getStageId(), $user->getId())
			) {
				// Authors are not permitted to revise reviewer documents.
				continue;
			}
			$submissionFiles[] = $submissionFile;
		}

Logic in 3.4

Same as in 3.3, just migrated from getBySubmissionAndRoleId! to getBySubmissionAndRoleIds

        foreach ($allSubmissionFiles as $submissionFile) {
            // The uploaded file must be excluded from the list of revisable files.
            if ($uploadedFile && $uploadedFile->getId() == $submissionFile->getId()) {
                error_log('continue 1');
                continue;
            }
            if (
                ($submissionFile->getFileStage() == SubmissionFile::SUBMISSION_FILE_REVIEW_ATTACHMENT || $submissionFile->getFileStage() == SubmissionFile::SUBMISSION_FILE_REVIEW_FILE) &&
                $stageAssignmentDao->getBySubmissionAndRoleIds($submissionFile->getData('submissionId'), [Role::ROLE_ID_AUTHOR], $this->getStageId(), $user->getId())
            ) {
                // Authors are not permitted to revise reviewer documents.
                
                continue;
            }
            $submissionFiles[] = $submissionFile;
        }

Logic in 3.5

In 3.5 list of files to revise shows up for editor, because of the refactor of the StageAssignments which fixed previous approach

        foreach ($allSubmissionFiles as $submissionFile) {
            // The uploaded file must be excluded from the list of revisable files.
            if ($uploadedFile && $uploadedFile->getId() == $submissionFile->getId()) {
                continue;
            }
            // Replaces StageAssignmentDAO::getBySubmissionAndRoleIds
            $hasAnyAssignments = StageAssignment::withSubmissionIds([$submissionFile->getData('submissionId')])
                ->withRoleIds([Role::ROLE_ID_AUTHOR])
                ->withStageIds([$this->getStageId()])
                ->withUserId($user->getId())
                ->exists();

            if (
                ($submissionFile->getFileStage() == SubmissionFile::SUBMISSION_FILE_REVIEW_ATTACHMENT || $submissionFile->getFileStage() == SubmissionFile::SUBMISSION_FILE_REVIEW_FILE) &&
                $hasAnyAssignments
            ) {
                // Authors are not permitted to revise reviewer documents.
                continue;
            }
            $submissionFiles[] = $submissionFile;
        }

[bozana]

I think when uploading a file revision ("If you are uploading a revision of an existing file, please indicate which file." ) only the files from that grid needs to be considered, listed and provided for selection. This seems to be wrong in the review stage.
There is a difference between "If you are uploading a revision of an existing file, please indicate which file." and Revisions grid.
And authors should only see the Revisions grid, and no other files, no files from Review Files grid.

[jardakotesovec]

Oki, logic to show only revision of existing files from the same grid makes sense to me. For some reason it seems that for review stage that line was intentionally blurred. But its not working well (uploading in one grid, but showing up in another). So I think sticking with what you suggested make sense at this point.

Starting with PR for main branch, will back port same logic after the review and testing of this one:

main
pkp-lib: #11342 (merged)
ojs: (only tests) pkp/ojs#4856

3.5
pkp-lib: #11361 (same as main,merged)

3.4
ojs (only tests): pkp/ojs#4867
pkp-lib: #11341 (merged)

3.3
omp(only tests): pkp/omp#1980
pkp-lib: #11362 (merged)

[jardakotesovec]

@Tribunal33 Its now merged to main branch - can you stress test it little bit? :-) After that I would backport it to 3.5, 3.4 and 3.3.

[Tribunal33]

I've been trying @jardakotesovec but main is a bit broken. I kept getting errors. I will try again today as I see a fix is available.

[Tribunal33]

Looks good. There is a slightly unrelated issue if the participant is the author but also has another role and is added to the participant list. When logged in as the author or volume editor (for example) you will be restricted access to the file upload workflow. I think related to this #11223

Like this

Will show this when you try to upload

[Tribunal33]

I believe this is working as expected now. The orginal bug, however, is no longer able to complete the steps because the editor no longer is able to use the upload button in the revisions table to select files from the other table using the dropdown (Review Files). If an editor wants to upload files to re revisions table it will need to upload them manually. The author will not be able to revise this file due to authorization issues replacing the editor's file but will need to upload thier own file which will now be appropriately placed into the revisions table.

A potential improvement for the furture would be to either allow the author to revise the manually uploaded file the editor uploads in the revisions table or remove the editor's files from the drop down list when the author attempts to upload revisions.

@jardakotesovec does this sound correct?