pkp/pkp-lib#11779 added API endpoint to create task templates #11807

Hafsa-Naeem · 2025-09-10T12:59:34Z

Vitaliy-1

Thanks! I left a couple of comments

Vitaliy-1 · 2025-09-10T13:20:21Z

api/v1/editTaskTemplates/PKPEditTaskTemplateController.php

+            $payload['dateDue'] = $illuminateRequest->input('dateDue');
+        }
+
+        // validation


You can use the formRequest for validation https://github.com/pkp/pkp-lib/blob/main/api/v1/reviewers/suggestions/formRequests/AddReviewerSuggestion.php
Then, type hint it as an argument in the Controller method https://github.com/pkp/pkp-lib/blob/main/api/v1/reviewers/suggestions/ReviewerSuggestionController.php#L155

Vitaliy-1 · 2025-09-10T13:22:21Z

api/v1/editTaskTemplates/PKPEditTaskTemplateController.php

+            $template->userGroups()->sync($payload['userGroupIds']);
+
+            // store extra template config in settings (non-localized)
+            DB::table('edit_task_template_settings')->insert([


I don't think we have got any settings for the Task Template

Vitaliy-1 · 2025-09-10T13:25:55Z

api/v1/editTaskTemplates/PKPEditTaskTemplateController.php

+            return $template->getKey();
+        });
+
+        // serialize with TaskResource


Regarding Resources, it should have its own resource. You can use task resource as an example: https://github.com/pkp/pkp-lib/blob/main/api/v1/submissions/resources/TaskResource.php

Vitaliy-1

Thanks! Only 2 minor comments, otherwise looks good!

Vitaliy-1 · 2025-09-12T13:32:12Z

api/v1/editTaskTemplates/formRequests/AddEditTaskTemplate.php

+        $contextId = Application::get()->getRequest()->getContext()->getId();
+
+        return [
+            'stageId' => ['required', 'integer', 'min:1'],


you can check if the application has the given stage by Application::getApplicationStages()

Vitaliy-1 · 2025-09-12T13:43:11Z

api/v1/editTaskTemplates/formRequests/AddEditTaskTemplate.php

+            'stageId' => ['required', 'integer', 'min:1'],
+            'title' => ['required', 'string', 'max:255'],
+            'include' => ['sometimes', 'boolean'],
+            'emailTemplateId' => ['sometimes', 'nullable', 'integer', Rule::exists('email_templates', 'email_id')],


Maybe specifying the key would be better?
And then we can validate with a custom rule: https://laravel.com/docs/12.x/validation#using-rule-objects. Trying to find all templates by their keys in the given context:

$templates = Repo::emailTemplate()->getCollector()->getMany();

Vitaliy-1

Thanks! A couple of additional comments

Vitaliy-1 · 2025-09-17T13:54:48Z

api/v1/editTaskTemplates/PKPEditTaskTemplateController.php

+            $emailTemplateId = null;
+            if (!empty($validated['emailTemplateKey'])) {
+                $et = Repo::emailTemplate()->getByKey($context->getId(), $validated['emailTemplateKey']);
+                $emailTemplateId = $et?->getId();


I think we should make an additional migration and reference from this column email template key, which also is unique. That's how later we can get either the default template, the customised one or a brand new, created specifically for the task

Vitaliy-1 · 2025-09-17T13:57:37Z

api/v1/editTaskTemplates/PKPEditTaskTemplateController.php

+        $template = DB::transaction(function () use ($validated, $context) {
+            $emailTemplateId = null;
+            if (!empty($validated['emailTemplateKey'])) {
+                $et = Repo::emailTemplate()->getByKey($context->getId(), $validated['emailTemplateKey']);


I think we can use one of the validation hooks instead of the controller for such actions. Not sure but it looks like we can create a method in the class that extends FormRequest for this:

protected function passedValidation() { // transform data after validation is passed }

Vitaliy-1 · 2025-09-17T13:59:39Z

api/v1/editTaskTemplates/formRequests/AddEditTaskTemplate.php

+        ]);
+    }
+
+    public function validated($key = null, $default = null)


Can you double check if we need to override this method here? I think we can avoid that...

Vitaliy-1 · 2025-09-17T14:05:58Z

api/v1/editTaskTemplates/resources/EditTaskTemplateResource.php

+            'userGroupIds' => $this->whenLoaded('userGroups', fn () => $this->userGroups->pluck('user_group_id')->values()->all()),
+            'userGroups' => $this->whenLoaded('userGroups', fn () => $this->userGroups->map(fn ($ug) => [
+                'id' => (int) $ug->user_group_id,
+                'name' => method_exists($ug, 'getLocalizedName') ? $ug->getLocalizedName() : ($ug->name ?? null),


Can you double check if we need to return an assoc array with data in all locales or just a localized string?

…ntroller

Vitaliy-1

Just minor comments, looks good, thanks!

Vitaliy-1 · 2025-09-23T12:13:56Z

api/v1/editTaskTemplates/formRequests/AddEditTaskTemplate.php

+use Illuminate\Foundation\Http\FormRequest;
+use Illuminate\Validation\Rule;
+
+class AddEditTaskTemplate extends FormRequest


It can be renamed just to the AddTaskTemplate to avoid confusion

Vitaliy-1 · 2025-09-23T12:22:41Z

classes/migration/upgrade/v3_6_0/I11779_UseEmailTemplateKeyOnEditTaskTemplates.php

+    public function up(): void
+    {
+        Schema::table('edit_task_templates', function (Blueprint $table) {
+            if (!Schema::hasColumn('edit_task_templates', 'email_template_key')) {


Personally, I'd avoid using such checks. That's because of something like this goes wrong, it's better to end the migration and give user the ability to restart it from the backup

Vitaliy-1 · 2025-09-23T12:22:52Z

classes/migration/upgrade/v3_6_0/I11779_UseEmailTemplateKeyOnEditTaskTemplates.php

+
+        // drop FK and old column
+        Schema::table('edit_task_templates', function (Blueprint $table) {
+            try { $table->dropForeign(['email_template_id']); } catch (\Throwable $e) {}


The same as above

Vitaliy-1 requested changes Sep 10, 2025

View reviewed changes

Hafsa-Naeem force-pushed the i11779-main-fix branch 2 times, most recently from 95e3d45 to d01209f Compare September 12, 2025 01:36

Hafsa-Naeem mentioned this pull request Sep 12, 2025

Add API endpoint to create task templates #11779

Closed

Vitaliy-1 requested changes Sep 12, 2025

View reviewed changes

Hafsa-Naeem force-pushed the i11779-main-fix branch from d01209f to 883a7ac Compare September 17, 2025 05:51

Vitaliy-1 requested changes Sep 17, 2025

View reviewed changes

Hafsa-Naeem added 3 commits September 23, 2025 05:06

pkp#11779 added API endpoint to create task templates

d5ff558

pkp#11779 Use FormRequest and templateResource for EditTaskTemplateCo…

0bd8bec

…ntroller

pkp#11779 Validate stageId and use emailTemplateKey

d47e557

Hafsa-Naeem force-pushed the i11779-main-fix branch from 883a7ac to df710af Compare September 23, 2025 02:00

pkp#11779 Refactor task template to use email_template_key instead of Id

f25b7a9

Hafsa-Naeem force-pushed the i11779-main-fix branch from df710af to f25b7a9 Compare September 23, 2025 02:02

Vitaliy-1 approved these changes Sep 23, 2025

View reviewed changes

Vitaliy-1 closed this Sep 29, 2025

pkp/pkp-lib#11779 added API endpoint to create task templates #11807

pkp/pkp-lib#11779 added API endpoint to create task templates #11807

Uh oh!

Conversation

Hafsa-Naeem commented Sep 10, 2025

Uh oh!

Vitaliy-1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Vitaliy-1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Vitaliy-1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Vitaliy-1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!