Fatal error when exporting submission with the Native XML

[jonasraoni]

Describe the bug
When trying to export a submission file that doesn't have an uploaderUserId, the following error happens:

PKP\user\Repository::get(): Argument #1 ($id) must be of type int, null given, called in /var/www/html/lib/pkp/plugins/importexport/native/filter/SubmissionFileNativeXmlFilter.php on line 86

This is related to #8493, and there are other places in the codebase that require attention.

In case this field didn't exist in the past (e.g. OJS 2.x) or if it's related to bugs, an alternative fix would be assign the admin user to the null cells on a next upgrade (3.5).

What application are you using?
OJS 3.4

[asmecher]

@defstat, passing to you to consider for quality-of-life import/export issues.

[jonasraoni]

Regarding the uploaderUserId, I'm just curious if it should be a not null field. There's a script to ensure it's filled with something here: https://github.com/pkp/ojs/blob/1c2ff7b057434e55eeedfb8b4c91087aa7852458/classes/install/Upgrade.inc.php#L68-L94

[defstat]

I'am, maybe, stating something obvious here, but I want to clear that in my head mostly: I think that the uploaderUserId of submissionFile can be one of the three states bellow:

1. The creation of the submission file is done as part of the initial upload that happens in the context of a "user session" in the submission's workflow.
2. The creation of the submission file is done as part of an import tool
   • (a) The imported submissionFile has an uploaderUserId defined (or a username defined in the case of native import/export) in it's dataset and this uploaderUserId can be found in the target DB
   • (b) The imported submissionFile has an uploaderUserId defined (or a username defined in the case of native import/export) in it's dataset and this uploaderUserId can not be found in the target DB
   • (c) The imported submissionFile has not an uploaderUserId defined (or a username defined in the case of native import/export)
3. The creation of the submission file is done as part of another process that does not involve a user (not sure if there is such a case)

For the 1. above, the submission file should get the context's userId and add that to the submission file's uploaderUserId. The corresponding submissionFile should always have an uploaderUserId.

For the 2. above - a general comment: I am wondering what is the correct way to deal with that taking into account that the user that executes the process, thus the creation of the submissionFile, is not necessarily (and in most of the cases he/she is not) the uploaderUserId that can be found in the dataset of the imported data. Another consideration here is whether we expect the uploaderUserId or the username defined in the dataset of the imported data to match, without a doubt, the corresponding user (if any) at the target installation.

Technically adding one, non globally acceptable identifier (the OJS's id or even the username are not globally acceptable identifiers) is not sufficient to match the user. Maybe adding more data for the user (name, surname, ORCID, etc) may decrease the possibility of a wrong user matching, but it will increase the possibility of not matching the user to an existing one at the target installation.

If we add the id of actual user that uses the import tool as uploaderUserId in all the created submissionFiles bound to this import process then we will, on the one hand, be sure that an uploaderUserId will be assigned to the imported submission file, but any possible exported data regarding the initial uploaderUserId will be lost.

There could be a mixed approach to that, namely add the found uploaderUserId if there is any or the id of the user that executes the import tool, if there is not any user matching the uploaderUserId.

Generally I would advocate that always/consistently keeping the actual creator id of the "current" installation makes more sense to me, than "migrating" a user id or username from an installation to another one for "systemic data" such as the uploaderUserId.

For the 3. above - If there is such a case, we might consider a "system user" "special" id that does not correspond to a real user - something equivalent to the PKPApplication::CONTEXT_ID_NONE.

[jonasraoni]

As another user had the problem, I've just uploaded a fast-fix (the code was already expecting an empty uploader).

[jonasraoni]

Regarding the discussion:
In my opinion the uploader user ID can be removed from the NativeXML input/output.
For the end user: I guess it's not important who uploaded the file, they have the authors.
For a system admin: It's important to know who uploaded an unsafe file.

Then about the fix:

• I think we can re-run this migration: https://github.com/pkp/ojs/blob/1c2ff7b057434e55eeedfb8b4c91087aa7852458/classes/install/Upgrade.inc.php#L68-L94
• Update the code to always expect that an uploader exists (a couple of places are checking if it exists).
• Move the data from the settings table to a physical field and make it not null (extra consideration will be needed when deleting a user from the system, perhaps we're just "merging" users instead of deleting).

[defstat]

@jonasraoni thanks. I think this is already fixed in the codebase for Native Import/Export Plugin.

Before closing this issue I will tag @bozana in order to see if another issue should be opened regarding the uploaderId as it is described above.

@bozana, @jonasraoni your thoughts on that?