-
Notifications
You must be signed in to change notification settings - Fork 466
pkp/pkp-lib#11833 added API endpoint to get task templates #11834
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
Conversation
5091822
to
cd81835
Compare
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, I left a couple of comments
/** | ||
* GET /api/v1/editTaskTemplates | ||
*/ | ||
public function index(ListTaskTemplates $illuminateRequest): JsonResponse |
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's better to rename to getMany()
public function index(ListTaskTemplates $illuminateRequest): JsonResponse | ||
{ | ||
$context = $this->getRequest()->getContext(); | ||
$validated = $illuminateRequest->validated(); |
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 don't think we need any validation here. It's a get request for the list of templates
} | ||
|
||
$items = $q->orderByPkDesc()->get() | ||
->map(fn ($tpl) => (new EditTaskTemplateResource($tpl))->toArray($illuminateRequest)) |
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.
We need to pass a collection of templates instead of a single resource:
EditTaskTemplateResource::collection(Template::query()...->get())
use Illuminate\Foundation\Http\FormRequest; | ||
use Illuminate\Validation\Rule; | ||
|
||
class ListTaskTemplates extends FormRequest |
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.
We can just re-use already created class that extends FormRequest, and get requests usually don't require any validation. It's relevant for operations like create and edit
3803e23
to
3036c1a
Compare
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.
One small comment and it's ready to go! Thanks.
{ | ||
$context = $this->getRequest()->getContext(); | ||
|
||
$q = Template::query() |
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's better to use our already established pattern and scopes to avoid snake case props
$collector = Template::byContextId((int) $context->getId())->with('userGroups');
foreach ($request->query() as $param => $val) {
switch ($param) {
case 'stageId':
$collector = $collector->withStageId((int) $val);
break;
...
}
}
@Hafsa-Naeem, is any help here required? Seems like a small tweak an it's ready to go. |
2b71e4c
to
3e157ef
Compare
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! I've added a couple of comments
|
||
// Apply supported filters from query params via model scopes | ||
foreach ($request->query() as $param => $val) { | ||
switch ($param) { |
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.
Can you take a look at the example of the params filtering in the submission controller: https://github.com/pkp/pkp-lib/blob/main/api/v1/submissions/PKPSubmissionController.php#L473?
use Illuminate\Http\Request; | ||
use Illuminate\Http\Resources\Json\JsonResource; | ||
|
||
class EditTaskTemplateResource extends JsonResource |
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.
This is already added in the main: https://github.com/pkp/pkp-lib/blob/main/api/v1/editTaskTemplates/resources/TaskTemplateResource.php
…camelCase params
ba32576
to
3c8a6b1
Compare
for #11833