[PKP-LIB][main] #7505 Initial Commit Upload - Delete - Download JATS File #9536

defstat · 2023-11-24T10:44:55Z

No description provided.

asmecher

Hi @defstat! A handful of comments, mostly minor. Looking very good!

asmecher · 2023-11-24T22:56:46Z

classes/submissionFile/Repository.php

+    /**
+     * Function to get the Context's genres
+     */
+    public function getGenres(int $contextId) : Collection 


I think calling code should stick with the GenreDAO until that's adapted to a newer storage pattern; getGenres shouldn't be a method of this Repository class and it just spreads competing patterns through the codebase.

api/v1/submissions/PKPSubmissionController.php

classes/submissionFile/Repository.php

classes/components/listPanels/JatsListPanel.php

classes/db/DAOResultFactory.php

asmecher · 2023-11-24T23:51:02Z

classes/submissionFile/Repository.php

+     */
+    public function getDefaultJatsFileName(int $submissionId, int $publicationId): string
+    {
+        return 'jats-' . $submissionId . '-' . $publicationId . '-' . (string) mt_rand(100000, 999999) . '.xml';


This is just a download filename, right? Better to use a timestamp than a random number.

Changed it to return 'jats-' . $submissionId . '-' . $publicationId . '-' . date('Ymd-His') . '.xml';

api/v1/submissions/PKPSubmissionController.php

classes/submissionFile/Repository.php

schemas/submissionFile.json

asmecher

Looks very close, @defstat, thanks! I just made a few rearrangement suggestions.

api/v1/jats/PKPJatsController.php

classes/submissionFile/Repository.php

asmecher · 2023-12-05T22:46:38Z

classes/jats/Repository.php

@@ -0,0 +1,221 @@
+<?php
+/**
+ * @file classes/jats/Repository.php


I'm ambivalent about the use of a repository class for this. Actually, almost everything here is closely tied to the handling code, and will only be called once from the PKPJatsController -- so I'd be happy if it these functions were moved into PKPJatsController and this controller was removed. Then classes/jats/JatsFile.php could be moved into lib/pkp/classes/submissionFile alongside the parent class, which makes more sense to me.

But if you're not sure that's the best approach, I'd be OK to merge this as is too.

I had the relationship with the SubmissionFile and the SubmissionFile/Repository dropped for JatsFile object, and that's why I took both files out of the SubmissionFile folder and in it's own jats folder. This can be used as the "code home" for existing business logic and future possible merging of the code from, let's say, jatsTemplate plugin for generating the "default" Jats content.

asmecher · 2023-12-07T00:20:55Z

api/v1/jats/PKPJatsController.php

+
+        if ($actionName === 'add') {
+            $params = $illuminateRequest->input();
+            $fileStage = isset($params['fileStage']) ? (int) $params['fileStage'] : 21;


File stage shouldn't be hard-coded (21).

Changed to SubmissionFile::SUBMISSION_FILE_JATS

asmecher · 2023-12-07T00:21:12Z

api/v1/jats/PKPJatsController.php

+            ]),
+        ])->group(function () {
+
+            Route::get('', $this->get(...))


Probably my ignorance about Laravel's routes, but should these routes actually be empty strings?

You are right this parameter is reserved for the route declaration.

In our context the endpoint URL is constructed with the base endpoint Pattern that is defined here which takes into account this declaration.

Then the route defined in each Route::get('', $this->get(...)) is added after the base endpoint pattern.

All the endpoints for the specific Controller are api/submissions/{submissionId}/publications/{publicationId}/jats, which is the "Base URL" defined here. So the respective Route::get('', $this->get(...)) don't need to declare some url other than ''.

asmecher · 2023-12-07T00:22:59Z

classes/submissionFile/Repository.php

+     */
+    public function getSubmissionFileContent(SubmissionFile $submissionFile): string
+    {
+        $jatsFileName = Config::getVar('files', 'files_dir') . '/' . $submissionFile->getData('path') .'';


Change $jatsFileName to just $fileName, as this can be used for non-JATS files; also remove the empty string concatenation at the end.

Sure! Changed.

asmecher · 2023-12-07T00:23:34Z

classes/submissionFile/Repository.php

+    public function getSubmissionFileContent(SubmissionFile $submissionFile): string
+    {
+        $jatsFileName = Config::getVar('files', 'files_dir') . '/' . $submissionFile->getData('path') .'';
+        $fileContentString = file_get_contents($jatsFileName);


(Just return file_get_contents($fileName) would be a little more concise, since you're here anyway!)

jardakotesovec · 2023-12-07T10:06:47Z

@asmecher Are we confident that we want to have dependency from pkp-lib to jats plugin? Is not that opportunity to actually include jats plugin as part of the core if core functionality is dependent on it?

Update: now I actually see that jats is not even part of "included plugins".. And when I tried to check out branches 7505 branches for ojs, pkp and ui-library.

I am getting following error, when loading workflow page for any submission:

 Error: Class "APP\plugins\generic\jatsTemplate\classes\Article" not found in /Users/jarda/git/pkp/pkp-main/ojs-main/lib/pkp/classes/jats/Repository.php:199

asmecher · 2023-12-07T15:31:56Z

@asmecher Are we confident that we want to have dependency from pkp-lib to jats plugin?

Yes, by the time this is released I think we'll have moved the JATS Template plugin into core.

I am getting following error, when loading workflow page for any submission...

@defstat, could you look at that?

jardakotesovec · 2023-12-07T15:36:38Z

@asmecher @defstat Just to clarify. Cause of this problem is that I did not have JATS template plugin installed. Once I did clone that it works fine.. but it means that it would break workflow page for anyone who does not install JATS template plugin after merging.

defstat · 2023-12-07T15:39:26Z

I am getting following error, when loading workflow page for any submission:

@jardakotesovec I suppose that you don't have the jatsTemplate plugin in your installation.

I could perhaps catch any JATS creation error and display something different/special in that case - something like "Make sure the JatsTemplate plugin is installed" or something.

defstat · 2023-12-07T19:57:56Z

I could perhaps catch any JATS creation error and display something different/special in that case - something like "Make sure the JatsTemplate plugin is installed" or something.

@asmecher I added some code that returns custom exceptions in the case of

Failure of creating the default JATS content
Failure of reading the content of the provided Submission File.

In those case the UI is changed and some error message is displayed instead of the JATS content (see bellow)

Image 1 - Failure to create the Default JATS Content

In this case the user can still upload a locally stored file if so chooses

Image 2 - Failure to read the provided submission file

In this case the user can still delete the submission file.

jardakotesovec · 2023-12-12T13:07:58Z

js/load.js

@@ -198,6 +201,9 @@ VueRegistry.registerComponent('field-pub-id', FieldPubId);
 // Register ListPanel
 VueRegistry.registerComponent('PkpListPanel', ListPanel);

+// Register PublicationSectionJats
+VueRegistry.registerComponent('PkpPublicationJats', PublicationSectionJats);
+


Lets not register this component here. Here are only general components, so its possible to use them from plugins. Its perfectly sufficients that its 'components' field in workflow.vue

@jardakotesovec if I don't register the component here, how can I use the

<pkp-publication-jats v-bind="components.jats" class="pkpWorkflow__jats" @set="set" :publication="workingPublication" :publication-api-url="submissionApiUrl + '/publications/' + workingPublication.id" ></pkp-publication-jats>

in workflow.tpl?

I think I found it - I can put publication-section-jats instead of pkp-publication-jats because I have named the component PublicationSectionJats in the WorkflowPage.vue, and then remove the lines tagged from load.js

Fixed.

asmecher · 2023-12-12T19:40:48Z

Thanks, @defstat, I'm OK with the use of custom exceptions in this code. (I'd like to brainstorm a little with the team before we go too wild creating custom exception classes, because they are really going to multiply if the pattern is used in general! But it's OK here.)

…TS File (pkp#9536) * pkp#7505 Support for JATS files/content added * pkp#7505 Review Fixes * pkp#7505 Remove locale keys as vue component props * pkp#7505 Remove new Jats Component dependency from ListPanel * pkp#7505 Handle Jats creation Errors * pkp#7505 Fix UI in error cases * pkp#7505 Remove JATS Component from PKPWorkflow * pkp#7505 Review Changes

defstat mentioned this pull request Nov 24, 2023

Add basic support for JATS XML files to publications #7505

Closed

asmecher requested changes Nov 24, 2023

View reviewed changes

asmecher reviewed Nov 25, 2023

View reviewed changes

schemas/submissionFile.json Show resolved Hide resolved

defstat force-pushed the i7505 branch from 2e96228 to e7df19f Compare December 4, 2023 16:24

asmecher requested changes Dec 5, 2023

View reviewed changes

defstat force-pushed the i7505 branch 2 times, most recently from 1e83f2d to 3ba9171 Compare December 6, 2023 20:18

asmecher reviewed Dec 7, 2023

View reviewed changes

jardakotesovec reviewed Dec 12, 2023

View reviewed changes

defstat added 7 commits December 18, 2023 20:00

pkp#7505 Support for JATS files/content added

4bf44ab

pkp#7505 Review Fixes

0444142

pkp#7505 Remove locale keys as vue component props

3f37cdc

pkp#7505 Remove new Jats Component dependency from ListPanel

43f269c

pkp#7505 Handle Jats creation Errors

18419a7

pkp#7505 Fix UI in error cases

19cc4d3

pkp#7505 Remove JATS Component from PKPWorkflow

bcf378e

defstat force-pushed the i7505 branch from 7056f52 to bcf378e Compare December 18, 2023 18:01

pkp#7505 Review Changes

938de71

asmecher merged commit af8ad0f into pkp:main Dec 18, 2023
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PKP-LIB][main] #7505 Initial Commit Upload - Delete - Download JATS File #9536

[PKP-LIB][main] #7505 Initial Commit Upload - Delete - Download JATS File #9536

defstat commented Nov 24, 2023

asmecher left a comment

asmecher Nov 24, 2023

defstat Nov 25, 2023

asmecher Nov 24, 2023

defstat Nov 25, 2023

asmecher left a comment

asmecher Dec 5, 2023

defstat Dec 6, 2023

asmecher Dec 7, 2023

defstat Dec 7, 2023

asmecher Dec 7, 2023

defstat Dec 7, 2023 •

edited

Loading

asmecher Dec 7, 2023

defstat Dec 7, 2023 •

edited

Loading

asmecher Dec 7, 2023 •

edited

Loading

defstat Dec 7, 2023

jardakotesovec commented Dec 7, 2023 •

edited

Loading

asmecher commented Dec 7, 2023

jardakotesovec commented Dec 7, 2023

defstat commented Dec 7, 2023 •

edited

Loading

defstat commented Dec 7, 2023

jardakotesovec Dec 12, 2023

defstat Dec 18, 2023

defstat Dec 18, 2023 •

edited

Loading

asmecher commented Dec 12, 2023

[PKP-LIB][main] #7505 Initial Commit Upload - Delete - Download JATS File #9536

[PKP-LIB][main] #7505 Initial Commit Upload - Delete - Download JATS File #9536

Conversation

defstat commented Nov 24, 2023

asmecher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asmecher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

defstat Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

defstat Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

asmecher Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jardakotesovec commented Dec 7, 2023 • edited Loading

asmecher commented Dec 7, 2023

jardakotesovec commented Dec 7, 2023

defstat commented Dec 7, 2023 • edited Loading

defstat commented Dec 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

defstat Dec 18, 2023 • edited Loading

Choose a reason for hiding this comment

asmecher commented Dec 12, 2023

defstat Dec 7, 2023 •

edited

Loading

defstat Dec 7, 2023 •

edited

Loading

asmecher Dec 7, 2023 •

edited

Loading

jardakotesovec commented Dec 7, 2023 •

edited

Loading

defstat commented Dec 7, 2023 •

edited

Loading

defstat Dec 18, 2023 •

edited

Loading