-
Notifications
You must be signed in to change notification settings - Fork 15
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
[JatsTemplate][main] #7505 Initial Commit Upload - Delete - Download JATS File #41
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @defstat! Just a few questions/comments.
classes/Article.php
Outdated
$this->convertToXml($record); | ||
|
||
if ($record) { | ||
$this->convertToXml($record); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest moving this call outside of the constructor entirely; let the existing (JATSTemplatePlugin) use case call an additional function to perform the conversion, just like you've done for the new use case. (Call it e.g. convertToOai
.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My first instinct was also to remove it - on second thought, and because the constructor is being used in a way that I couldn't figure out why (but accepting that it has a reason), I followed this "safe" way.
I will refactor it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
classes/Article.php
Outdated
@@ -37,25 +41,34 @@ public function convertToXml($record) :bool|\DOMDocument | |||
$publication = $submission->getCurrentPublication(); | |||
$request = Application::get()->getRequest(); | |||
|
|||
$convertedXML = $this->convertSubmission($submission, $journal, $section, $issue, $publication, $request); | |||
|
|||
return $this->loadXml($convertedXML); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to cut down on the load/save cycle here; I'm not sure why it was written that way, but I don't think it's necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
classes/ArticleFront.php
Outdated
if ($datePublished) $datePublished = strtotime($datePublished); | ||
|
||
if (!$datePublished) $datePublished = strtotime('0001-01-01'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to properly indicate to the JATS output that we don't yet have a publication date?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this. I will refactor the JATS Template
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
Right now if a publication is assigned to an issue but is not published, a publication date is added to JATS. Not sure if that is correct though - I would expect that if the publication is not yet published, the publication date should be null, and not retrieved by the assigned issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be possible to omit the pub-date
element if the content is not yet published. (JATS reference for article metadata)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The added pub-date-not-available
element is removed
still wondering about that
Right now if a publication is assigned to an issue but is not published, a publication date is added to JATS. Not sure if that is correct though - I would expect that if the publication is not yet published, the publication date should be null, and not retrieved by the assigned issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, the publication date should not be added if the issue is not yet published.
classes/ArticleFront.php
Outdated
|
||
$dispatcher = new Dispatcher(); | ||
$dispatcher->setApplication(Application::get()); | ||
$dispatcher->addRouterName('\APP\core\PageRouter', PKPApplication::ROUTE_PAGE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem here is that this is called from an API endpoint. The Application::getRequest()
returns an APIRouter does not seem to like the url creation with parts other than the necessary for the endpoints.
This was a way to bypass that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to use:
$router = $request->getRouter();
$dispatcher = $router->getDispatcher();
$url = $dispatcher->url(...)
...rather than instantiating and configuring your own dispatcher.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right.
Fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, @defstat!
4f1391a
to
01087dd
Compare
Closed in favor of #42 |
No description provided.