pkp/pkp-lib#10571 limit email template access by user groups #10581
Conversation
taslangraham
force-pushed
the
i10571_limit_template_access_by_sender_and_receiver_role
branch
5 times, most recently
from
0352bec
to
ba47e2b
Compare
taslangraham
force-pushed
the
i10571_limit_template_access_by_sender_and_receiver_role
branch
7 times, most recently
from
3cb222d
to
cf0580b
Compare
taslangraham
changed the title
taslangraham
force-pushed
the
i10571_limit_template_access_by_sender_and_receiver_role
branch
2 times, most recently
from
8b6f076
to
75b9ed1
Compare
…il templates within a mailable
taslangraham
force-pushed
the
i10571_limit_template_access_by_sender_and_receiver_role
branch
from
75b9ed1
to
405e376
Compare
| ->getMany() | ||
| ->getMany(); | ||
|
|
||
| $templates = Repo::emailTemplate() |
There was a problem hiding this comment.
This $templates variable is directly replacing the one above. Is that what you meant to do? If so, the above one should be removed.
| $emailTemplates->add($emailTemplate); | ||
| } | ||
| Repo::emailTemplate() | ||
| ->getCollector($context->getId()) | ||
| ->alternateTo([$this->mailable::getEmailTemplateKey()]) | ||
| ->getMany() | ||
| ->each(fn (EmailTemplate $e) => $emailTemplates->add($e)); | ||
| ->each(function (EmailTemplate $e) use ($context, $request, $emailTemplates) { |
There was a problem hiding this comment.
Just a suggestion, but since this isn't an arrow function anymore, I would be more verbose about the variable name, just to make it easier to read at a glance.
| }, | ||
| "assignableTemplateUserGroups": { | ||
| "type": "array", | ||
| "description": "List of User Groups that can be assigned to the template", |
| }, | ||
| "isUnrestricted": { | ||
| "type": "boolean", | ||
| "description": "Boolean indicating if an email template is available to all user groups within the roles associated with the template's mailable.", |
There was a problem hiding this comment.
Extra space between indicating and if.
| "type": "integer", | ||
| "apiSummary": true | ||
| }, | ||
| "name": { |
There was a problem hiding this comment.
I'm not sure if the individual items support descriptions off the top of my head, but the name being an integer is a little confusing. Assuming this is correct, a description would be helpful to understand what the number is supposed to represent in terms of the name.
There was a problem hiding this comment.
That's an error on my part. It should be a string.
| @@ -46,6 +48,14 @@ public function __construct(string $action, array $locales) | |||
| 'isMultilingual' => true, | |||
| 'toolbar' => 'bold italic superscript subscript | link | blockquote bullist numlist', | |||
| 'plugins' => 'paste,link,lists', | |||
| ]))->addField(new FieldEmailTemplateUnrestricted('isUnrestricted', [ | |||
There was a problem hiding this comment.
I think a lot more of the setup logic for the Vue components can be done in PHP so that these can both be done with existing FieldOptions components. I mentioned this in the ui-library PR as well and we can chat about it more in our next one-on-one (probably easier to share screen and code together than writing it all out here).
| * | ||
| */ | ||
| public function installEmailTemplates( | ||
| string $templatesFile, | ||
| array $locales = [], | ||
| ?string $emailKey = null, | ||
| bool $skipExisting = false | ||
| bool $skipExisting = false, | ||
| $recordTemplateGroupAccess = false |
There was a problem hiding this comment.
I mention this directly below, but I don't think this optional parameter will be necessary. I think the assumption of "unrestricted if no additional data provided" below is enough.
| @@ -0,0 +1,34 @@ | |||
| <?php | |||
| @@ -0,0 +1,74 @@ | |||
| <?php | |||
| /** | ||
| * @file classes/migration/upgrade/v3_5_0/PreflightCheckMigration.php | ||
| * | ||
| * Copyright (c) 2014-2021 Simon Fraser University |
There was a problem hiding this comment.
Should be 2024 here and below.
For #10571