Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[OJS] Native Import/Export Plugin v3.2 #2646

Merged
merged 6 commits into from
Feb 28, 2020

Conversation

defstat
Copy link
Contributor

@defstat defstat commented Feb 26, 2020

No description provided.

Copy link
Member

@asmecher asmecher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking very good so far, @defstat!

$submissions = array();
foreach ($submissionIds as $submissionId) {
$submission = $submissionDao->getById($submissionId, $context->getId());
/** @var $submissionService APP\Services\SubmissionService */
$submissionService = Services::get('submission');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This removes the check to make sure that the submission is in the right context. I'd love to see this re-added upstream (e.g. in PKPSubmissionService::get) -- ping @NateWr -- but we probably need to talk about standardizing on that post-3.2. For now, please add a check here by hand, e.g.:

if ($submission->getData('contextId') != $context->getId()) continue;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd love to see this re-added upstream

I'd like to avoid this if we can, because it introduces inconsistencies in the EntityReadInterface, but let's chat about this post-3.2.

// */
// function getRepresentationExportFilterGroupName() {
// return 'article-galley=>native-xml';
// }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Dead code needs removal)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@@ -22,6 +22,8 @@ class AuthorNativeXmlFilter extends PKPAuthorNativeXmlFilter {
*/
function __construct($filterGroup) {
parent::__construct($filterGroup);

$this->getNoValidation(true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(This is temporary pending tomorrow's work, I think?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed


// Add public ID
if ($pubId = $issueGalley->getStoredPubId('publisher-id')) {
$issueGalleyNode->appendChild($node = $doc->createElementNS($deployment->getNamespace(), 'id', htmlspecialchars($pubId, ENT_COMPAT, 'UTF-8')));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's cleaner (and probably safer) to use DOMElement::createTextNode rather than htmlspecialchars to do the escaping. (Here and probably elsewhere)

@@ -69,7 +108,7 @@ function createCoversNode($filter, $doc, $object) {

import('classes.file.PublicFileManager');
$publicFileManager = new PublicFileManager();
$filePath = $publicFileManager->getContextFilesPath($object->getData('contextId')) . '/' . $coverImage;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(@NateWr is moving us away from getter functions and towards getData with the introduction of SchemaDAO -- so we may end up changing this back. But for now IssueDAO is not a SchemaDAO, so this is fine, just a heads-up.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

$publication->setData('sectionId', $section->getId());
}
}
// check if article is related to an issue, but has no published date
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this an be left out.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented out.

$importClass='ArticleGalley';
break;
default:
$importClass=null; // Suppress scrutinizer warn
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this be caught by XML validation? If so, there's no need to code for it here. Or it could be an assert, fatalError or Exception if it's a code path that should never run.

function parseIssueIdentification($publication, $node) {
$deployment = $this->getDeployment();
$context = $deployment->getContext();
// $submission = $deployment->getSubmission();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(remove dead code)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

* Constructor
* @param $filterGroup FilterGroup
*/
function __construct($filterGroup) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the dead constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

* Get the method name for inserting a published submission.
* @return string
*/
function getPublishedSubmissionInsertMethod() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this function used? I don't see it anywhere (and I think it's the same method name in all apps anyway).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@asmecher asmecher merged commit a63b3a8 into pkp:master Feb 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants