[OMP] [stable-3_4_0] Port the CSV importexport tool for OMP 3.4 #1828
Conversation
|
Hello @asmecher . Just a heads-up that I made the porting for this tool and it's ready to be reviewed! |
There was a problem hiding this comment.
Looks like this is sample data that should be excluded from the pull request
There was a problem hiding this comment.
Looks like this is sample data that should be excluded from the pull request
plugins/importexport/csv/classes/processors/PublicationFileProcessor.php
Outdated
Show resolved
Hide resolved
|
|
||
| class CachedEntities | ||
| { | ||
|
|
|
Hi @Godoy0722, I've added a few initial comments above. I also noticed that tabs are being used to indent in many files, but should be converted to spaces, and there are a few files with added blank lines at the end to remove. I was wondering about the decision to break the code down into many small classes with a single function (e.g. all of the processors here). To me it makes the code more difficult to read and maintain, but interested to know your take on it. |
|
Hello, @kaitlinnewson. I just updated the code to match your requests. I was trying to refactor the code in a way we could see the separated processings or at a later moment, reuse this code in different ways. But if you think it won't be necessary I can undo this refactoring and put it all inside the base code again and maybe keep only the validations in a different code. What do you think about it? |
plugins/importexport/csv/classes/validations/InvalidRowValidations.php
Outdated
Show resolved
Hide resolved
…ions.php Co-authored-by: Kaitlin Newson <[email protected]>
|
Hello @kaitlinnewson , just want to know if is there anything else you like to add or something else you'd like for me to do, or if is everything ok with this PR. |
| /** | ||
| * @copydoc Plugin::getDisplayName() | ||
| */ | ||
| function getDisplayName() { |
There was a problem hiding this comment.
Missing visibility (public) for function (and in more functions below).
| $bookCoverImageSaved = $this->fileManager->copyFile($srcFilePath, $destFilePath); | ||
|
|
||
| if (!$bookCoverImageSaved) { | ||
| $reason = __('plugin.importexport.csv.erroWhileSavingBookCoverImage'); |
There was a problem hiding this comment.
| $reason = __('plugin.importexport.csv.erroWhileSavingBookCoverImage'); | |
| $reason = __('plugin.importexport.csv.errorWhileSavingBookCoverImage'); |
| msgid "pugin.importexport.csv.missingHeadersOnCsv" | ||
| msgstr "The following headers are missing in your CSV file: \"{$missingHeaders}\". Please update it as shown in the sample.csv file." | ||
|
|
||
| msgid "plugin.importexport.csv.erroWhileSavingBookCoverImage" |
There was a problem hiding this comment.
| msgid "plugin.importexport.csv.erroWhileSavingBookCoverImage" | |
| msgid "plugin.importexport.csv.errorWhileSavingBookCoverImage" |
There was a problem hiding this comment.
A small formatting issue in this file - should be indented with tabs rather than spaces
|
Hi @Godoy0722, sorry for the delay in replying. I find that having many classes/functions (and a lot that are only used in one place) makes it a bit harder to understand the code, but I think a second review on this would be helpful from someone with more experience with these kinds of code design questions. |
|
Hi @jonasraoni, would you have time to do a second review on this? |
Suggested by Kaitlin, commited by Guilherme Co-authored-by: Kaitlin Newson <[email protected]>
Suggested by Kaitlin, commited by Guilherme Co-authored-by: Kaitlin Newson <[email protected]>
Describe the feature
This PR is intended to port the native CSV Importexport tool for submissions on OMP 3.4.