pkp/pkp-lib#11025 Include issue info on submission API data #4779

taslangraham · 2025-03-21T14:33:01Z

jonasraoni · 2025-03-21T16:10:13Z

classes/submission/maps/Schema.php

@@ -41,6 +44,9 @@

 class Schema extends \PKP\submission\maps\Schema
 {
+    /** Issues associated with submissions. Keyed by submission ID. */


Suggested change

/** Issues associated with submissions. Keyed by submission ID. */

/** @var Enumerable<int, Issue[]> Issues associated with submissions. Keyed by submission ID. **/

jonasraoni · 2025-03-21T16:12:12Z

classes/submission/maps/Schema.php

+
+    /**
+     * Get issues associated with submissions. Results are keyed by submission ID.
+     *


Not sure if it's keyed by submission ID 🙃

Suggested change

*

* @param Enumerable<int, Submission> $submissions Submissions, keyed by submission ID.

*

No, not required to be keyed by submission ID

jonasraoni · 2025-03-21T16:15:27Z

classes/submission/maps/Schema.php

+        $submissionIds = $submissions->map(fn (Submission $submission) => $submission->getId())->all();
+        $publications = Repo::publication()->getCollector()->filterBySubmissionIds($submissionIds)->getMany();
+        $issueIds = $publications->map(fn (Publication $publication) => $publication->getData('issueId'))
+            ->unique()
+            ->all();


I guess the Submission object is supposed to have data in the getData('publications'), which will be a list of publications, then the $submissionIds can be removed and the $publications can be optimized.

But I guess this is the class where the publications is populated, so I'm not sure if the data is available yet, if not, so ignore my comment 🤔

jonasraoni · 2025-03-21T16:23:26Z

classes/submission/maps/Schema.php

+
+        return $submissions->mapWithKeys(function ($submission) use (&$issues, $publications, $issueIdsGroupedBySubmission) {
+            $submissionIssueIds = $issueIdsGroupedBySubmission->get($submission->getId())->all();
+            return [$submission->getId() => $issues->filter(fn ($issue) => in_array($issue->getId(), $submissionIssueIds))];


I guess the $issues is keyed by issueId, then instead of filtering through all issues, followed by an in_array(), you can just collect the right ones.

I mean something like [$submission->getId() => $submissionIssueIds->map(fn (int $issueId) => $issues->get($issueId))

jonasraoni · 2025-03-21T16:24:19Z

classes/submission/maps/Schema.php

+        $issueIdsGroupedBySubmission = $publications->groupBy(fn (Publication $publication) => $publication->getData('submissionId'))
+            ->map(fn ($entry) => $entry->map(fn (Publication $publication) => $publication->getData('issueId')));
+
+        return $submissions->mapWithKeys(function ($submission) use (&$issues, $publications, $issueIdsGroupedBySubmission) {


The reference is not needed, the variable isn't updated inside the callback

Suggested change

return $submissions->mapWithKeys(function ($submission) use (&$issues, $publications, $issueIdsGroupedBySubmission) {

return $submissions->mapWithKeys(function ($submission) use ($issues, $publications, $issueIdsGroupedBySubmission) {

jonasraoni · 2025-03-21T16:26:39Z

classes/submission/maps/Schema.php

+    }
+
+    /**
+     * Get details about the issue a submission will be published in.


Suggested change

* Get details about the issue a submission will be published in.

* Get details about the issue a submission will be published in.

* @param Enumerable<int, Publication> $publications Publications, keyed by publication ID.

jonasraoni · 2025-03-21T16:26:47Z

classes/submission/maps/Schema.php

+        if (empty($this->submissionsIssues)) {
+            $this->submissionsIssues = $this->getSubmissionsIssues($submissions);
+        }


Suggested change

if (empty($this->submissionsIssues)) {

$this->submissionsIssues = $this->getSubmissionsIssues($submissions);

}

$this->submissionsIssues ??= $this->getSubmissionsIssues($submissions);

jonasraoni · 2025-03-21T16:27:47Z

classes/submission/maps/Schema.php

+    {
+        /** @var Publication $latestScheduledPublication */
+        $latestScheduledPublication = $publications
+            ->filter(fn ($publication) => $publication->getData('status') === Submission::STATUS_SCHEDULED)


I guess you just need the publications which are going to be published in the future, right? Otherwise, I think the STATUS_PUBLISHED should be included.

we are only interested in the scheduled publications

jonasraoni · 2025-03-21T16:30:05Z

classes/submission/maps/Schema.php

+            ->sortByDesc(fn (Publication $publication) => $publication->getData('version'))
+            ->first();
+
+        if ($latestScheduledPublication) {


I think this type of early return is better to read

Suggested change

if ($latestScheduledPublication) {

if (!$latestScheduledPublication) {

return null;

}

jonasraoni · 2025-03-21T16:31:26Z

classes/submission/maps/Schema.php

+    }
+
+    /**
+     * Populate class properties specific to OJS.


Suggested change

* Populate class properties specific to OJS.

* Populate class properties specific to OJS.

* @param Enumerable<int, Submission> $submissions Submissions, keyed by submission ID.

$submissions won't necessarily be keyed by ID, example when this method is called from map method in schema, like: $this->addAppSpecificData(collect([$item]));, that won't be keyed by submission ID

jonasraoni · 2025-03-21T16:47:48Z

classes/submission/maps/Schema.php

+        if ($latestScheduledPublication) {
+            $submissionId = $latestScheduledPublication->getData('submissionId');
+            $issueId = $latestScheduledPublication->getData('issueId');
+            $issue = $this->submissionsIssues->get($submissionId, collect())->get($issueId);


Perhaps:

Suggested change

$issue = $this->submissionsIssues->get($submissionId, collect())->get($issueId);

$issue = $this->submissionsIssues->get($submissionId)?->get($issueId);

jonasraoni

As a reader it's not clear for me that the $this->submissionsIssues will be filled implicitly, some time before the getPropertyIssueToBePublished() is called, so I decided to watch better this Schema class...

I see the purpose of most of the code in this PR, and also the PR for the pkp-lib, is mostly to pre-load the list of issues, which is good, but I think there's a simpler alternative.

Looks like $this->collection will be filled whenever a list of submissions is going to be mapped, so you can probably drop most of the code, and use a generic Issue loader on the getPropertyIssueToBePublished(), e.g.:

    private function getIssue(int $issueId): ?Issue
    {
        if ($this->cachedIssues === null) {
            if (!$this->collection) {
                $this->cachedIssues = collect([$issueId => Repo::issue()->get($issueId)]);
            } else {
                //code from the `getSubmissionsIssues()`
                $this->cachedIssues = ...;
            }
        }
        
        return $this->cachedIssues->get($issueId);
    }

taslangraham · 2025-03-24T18:45:48Z

As a reader it's not clear for me that the $this->submissionsIssues will be filled implicitly, some time before the getPropertyIssueToBePublished() is called, so I decided to watch better this Schema class...

I see the purpose of most of the code in this PR, and also the PR for the pkp-lib, is mostly to pre-load the list of issues, which is good, but I think there's a simpler alternative.

Looks like $this->collection will be filled whenever a list of submissions is going to be mapped, so you can probably drop most of the code, and use a generic Issue loader on the getPropertyIssueToBePublished(), e.g.:
    private function getIssue(int $issueId): ?Issue
    {
        if ($this->cachedIssues === null) {
            if (!$this->collection) {
                $this->cachedIssues = collect([$issueId => Repo::issue()->get($issueId)]);
            } else {
                //code from the `getSubmissionsIssues()`
                $this->cachedIssues = ...;
            }
        }
        
        return $this->cachedIssues->get($issueId);
    }

I think the suggested code kind of drifts away from the existing pattern of how data is being pre-loaded before being used in a getPropertyXYZ method throughout the schema. Best if this PR keeps consistent with how things are already being done.

taslangraham mentioned this pull request Mar 21, 2025

Redesigned Dashboard rc2 - API published in issue pkp/pkp-lib#11025

Closed

jonasraoni reviewed Mar 21, 2025

View reviewed changes

jonasraoni requested changes Mar 21, 2025

View reviewed changes

taslangraham force-pushed the i11025-main branch from cba5c3c to 5a37c18 Compare March 24, 2025 18:37

taslangraham requested a review from jonasraoni March 25, 2025 16:28

taslangraham added 2 commits March 27, 2025 11:58

pkp/pkp-lib#11025 Include issue info on submission API data

d25c07d

Submodule update

ebdbfeb

taslangraham force-pushed the i11025-main branch from 5a37c18 to ebdbfeb Compare March 27, 2025 17:00

taslangraham merged commit 90c9fcd into pkp:main Mar 27, 2025
7 of 8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkp/pkp-lib#11025 Include issue info on submission API data #4779

pkp/pkp-lib#11025 Include issue info on submission API data #4779

taslangraham commented Mar 21, 2025

jonasraoni Mar 21, 2025 •

edited

Loading

jonasraoni Mar 21, 2025 •

edited

Loading

taslangraham Mar 24, 2025

jonasraoni Mar 21, 2025

jonasraoni Mar 21, 2025

jonasraoni Mar 21, 2025

jonasraoni Mar 21, 2025

jonasraoni Mar 21, 2025 •

edited

Loading

jonasraoni Mar 21, 2025

jonasraoni Mar 21, 2025

taslangraham Mar 24, 2025

jonasraoni Mar 21, 2025

jonasraoni Mar 21, 2025

taslangraham Mar 24, 2025

jonasraoni Mar 21, 2025

jonasraoni left a comment •

edited

Loading

taslangraham commented Mar 24, 2025

	/** Issues associated with submissions. Keyed by submission ID. */
	/ @var Enumerable<int, Issue[]> Issues associated with submissions. Keyed by submission ID. /

	*
	* @param Enumerable<int, Submission> $submissions Submissions, keyed by submission ID.
	*

	return $submissions->mapWithKeys(function ($submission) use (&$issues, $publications, $issueIdsGroupedBySubmission) {
	return $submissions->mapWithKeys(function ($submission) use ($issues, $publications, $issueIdsGroupedBySubmission) {

	* Get details about the issue a submission will be published in.
	* Get details about the issue a submission will be published in.
	* @param Enumerable<int, Publication> $publications Publications, keyed by publication ID.

-        if ($latestScheduledPublication) {
+        if (!$latestScheduledPublication) {
+            return null;
+        }

	* Populate class properties specific to OJS.
	* Populate class properties specific to OJS.
	* @param Enumerable<int, Submission> $submissions Submissions, keyed by submission ID.

	$issue = $this->submissionsIssues->get($submissionId, collect())->get($issueId);
	$issue = $this->submissionsIssues->get($submissionId)?->get($issueId);

pkp/pkp-lib#11025 Include issue info on submission API data #4779

pkp/pkp-lib#11025 Include issue info on submission API data #4779

Conversation

taslangraham commented Mar 21, 2025

jonasraoni Mar 21, 2025 • edited Loading

Choose a reason for hiding this comment

jonasraoni Mar 21, 2025 • edited Loading

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

jonasraoni Mar 21, 2025 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonasraoni left a comment • edited Loading

Choose a reason for hiding this comment

taslangraham commented Mar 24, 2025

jonasraoni Mar 21, 2025 •

edited

Loading

jonasraoni Mar 21, 2025 •

edited

Loading

jonasraoni Mar 21, 2025 •

edited

Loading

jonasraoni left a comment •

edited

Loading