Multiple author affiliations #10460
Multiple author affiliations #10460
Conversation
| /** | ||
| * Set name | ||
| */ | ||
| public function setName(?array $name): void |
There was a problem hiding this comment.
I think we should stay consistent with other 'settings' method that could have locale and have something like:
public function setName(string|array $name, ?string $locale = null)
{
$this->setData('name', $name, $locale);
}
public function getName(?string $locale = null)
{
return $this->getData('name', $locale);
}
When you then provide an array as $name and no locale to setName() it will/should be set correctly.
| } | ||
| yield $row->author_affiliation_id => $fromRow; | ||
| } | ||
| }); |
There was a problem hiding this comment.
This is not easy to read and understand, so maybe to do it similarly to author DAO and set affiliations?
To get the affiliation fromRow() and then get the Ror by ID and set the affiliations names.
There was a problem hiding this comment.
Then we would also not need that function getManyRorAsCollectionId() in Ror Collector
| public function fromRow(object $row): Affiliation | ||
| { | ||
| return parent::fromRow($row); | ||
| } |
There was a problem hiding this comment.
This function would then get the ror names and set the affiliation names to be the same as ror names
|
|
||
| class Ror extends DataObject | ||
| { | ||
| public const int STATUS_ACTIVE = 1; |
There was a problem hiding this comment.
it should be public const STATUS_ACTIVE = 1; i.e. without the type int
here and below
| { | ||
| $this->authorIds = $authorIds; | ||
| return $this; | ||
| } |
There was a problem hiding this comment.
It seems this function is never used?
Then maybe to leave the filtering only by one author ID?
| public ?string $name = null; | ||
|
|
||
| /** Get affiliations with a searchPhrase */ | ||
| public ?string $searchPhrase = null; |
There was a problem hiding this comment.
It seems we do not consider this anywhere, so maybe to remove it for now.
| function (JoinClause $join) { | ||
| $join | ||
| ->on('r_s.setting_value', '=', 'a_s.setting_value') | ||
| ->on('r_s.locale', '=', 'a_s.locale') |
There was a problem hiding this comment.
does locale needs to be the same? 🤔
| ]; | ||
| $affiliation->setAllData($params); | ||
|
|
||
| Repo::affiliation()->dao->updateOrInsert($affiliation); |
There was a problem hiding this comment.
In upgrade scripts we are using only SQLs and no objects and functions -- so that the upgrade can be correctly done for the given version even if we change the objects and code.
Thus, we should try to use the query builder to insert or update the affiliations.
| ]; | ||
| $affiliation->setAllData($params); | ||
|
|
||
| Repo::affiliation()->dao->updateOrInsert($affiliation); |
There was a problem hiding this comment.
Here as well, we should use the query builder to insert or update the affiliation
| } | ||
| } | ||
|
|
||
| $affiliation->setAllData($params); |
There was a problem hiding this comment.
I think it is better to use setName, setRor and get rid of $params array.
(Then take care about the name, not to be overwritten.)
| */ | ||
| public function getByAuthorId(int $authorId, ?string $submissionLocale = null): array | ||
| { | ||
| return iterator_to_array( |
There was a problem hiding this comment.
instead of using the php function iterator_to_array we should use Laravel all() function:
$this->getCollector()
->filterByAuthorId($authorId)
->getMany($submissionLocale)
->all()
Here and elsewhere if similar cases exist...
|
|
||
| $params['id'] = $newId; | ||
| $author->setId($newId); | ||
| $author->setAffiliations($params['affiliations']); |
There was a problem hiding this comment.
is $params['affiliations'] array of Affiliation objects?
There was a problem hiding this comment.
I think you should here do:
- Repo::affiliation()->validate($author, $params, $submission, $submissionContext);
- if error -> return
- Repo::affiliation()->newDataObject($params);
- author->setAffiliations...
There was a problem hiding this comment.
Maybe also to do here:
foreach ($params['affiliations'] as $affiliationParam) {
then the steps above
and having Repo::affiliation()->validate(null, $params, $author, $submission, $submissionContext); i.e. validating only one affiliation i.e. having the first parameter Affiliation object -- see below where the validate is implemented.
There was a problem hiding this comment.
This also below, in the editContributor function
There was a problem hiding this comment.
Maybe to create Affiliation objects and use author->setAffiliations (steps 1 - 4 above) before adding the author.
Then in the author DAO the author is first added and then the affiliations saved, containing the author ID.
| @@ -1558,6 +1559,15 @@ public function addContributor(Request $illuminateRequest): JsonResponse | |||
|
|
|||
| $author = Repo::author()->newDataObject($params); | |||
| $newId = Repo::author()->add($author); | |||
|
|
|||
| $params['id'] = $newId; | |||
There was a problem hiding this comment.
should this be 'authorId' instead of 'id'?
| $validator = ValidatorFactory::make( | ||
| $props, | ||
| $schemaService->getValidationRules(PKPSchemaService::SCHEMA_AUTHOR, $allowedLocales) | ||
| ); |
There was a problem hiding this comment.
I do not think you need this
| $allowedLocales = $submission->getPublicationLanguages($context->getSupportedSubmissionMetadataLocales()); | ||
|
|
||
| // Check if author exists | ||
| if(!empty($props['id'])){ |
There was a problem hiding this comment.
can we call it authorId instead of id?
There was a problem hiding this comment.
maybe to use isset($props['id'])
| $schemaService->getValidationRules(PKPSchemaService::SCHEMA_AUTHOR, $allowedLocales) | ||
| ); | ||
| $validator->after(function ($validator) use ($props) { | ||
| if (isset($props['id']) && !$validator->errors()->get('id')) { |
There was a problem hiding this comment.
If you check above for isset($props['id']) then no need here
| * | ||
| * @hook Affiliation::validate [[&$errors, $object, $props, $submission, $context]] | ||
| */ | ||
| public function validate(?Author $author, array $props, Submission $submission, Context $context): array |
There was a problem hiding this comment.
Maybe to have the syntax as in all validate functions: to validate only one affiliation and to have the first parameter Affiliation object that could be null ?
| */ | ||
| public function saveAffiliations(Author $author): void | ||
| { | ||
| $affiliations = $author->getAffiliations(); |
There was a problem hiding this comment.
Here we can expect the affiliations to always be an array of Affiliation objects. (I commented above in PKPSubmissionController in add/editContributor)
| } | ||
|
|
||
| /** | ||
| * Get the localized affiliations as an array. |
There was a problem hiding this comment.
localized affiliations names
|
|
||
| foreach ($this->getAffiliations() as $affiliation) { | ||
| foreach ($this->getLocalePrecedence($preferredLocale) as $locale) { | ||
| $name = $affiliation->getData('name'); |
There was a problem hiding this comment.
the $locale parameter is missing here
| foreach ($this->getLocalePrecedence($preferredLocale) as $locale) { | ||
| $name = $affiliation->getData('name'); | ||
| if (!empty($name[$locale])) { | ||
| $value[] = $name[$locale]; |
There was a problem hiding this comment.
there should be a break, in order not to collect names in all the locales
| <complexType> | ||
| <sequence> | ||
| <element name="ror" type="string" minOccurs="0" maxOccurs="unbounded" /> | ||
| <element name="name" type="pkp:localizedNode" minOccurs="0" maxOccurs="unbounded" /> |
There was a problem hiding this comment.
Should the minOccurs for ror and name be 1 -- if there is a rorAffiliation, then both elements should exist, no?
| } | ||
| } | ||
| foreach($affiliations as $affiliation) { | ||
| if(!empty($affiliation->getData('ror'))) { |
There was a problem hiding this comment.
no need to have two times the same foreach($affiliations as $affiliation).
Maybe then to first check if(!empty($affiliation->getData('ror'))) { then add rorAffilaition element, else if (!empty($affiliation->getData('name') add affiliation element.
| $locale = $publication->getData('locale'); | ||
| } | ||
| $author->setAffiliation($n->textContent, $locale); | ||
| break; |
There was a problem hiding this comment.
I believe this should stay, and the new element rorAffiliation needs to be considered as well, no?
Maybe the only thing is: if we have the affiliation element, if we would like to see if the ROR exists for it, like in the upgrade script 🤔
| /** | ||
| * Get localized affiliations. | ||
| */ | ||
| public function getLocalizedAffiliations(?string $preferredLocale = null): array |
There was a problem hiding this comment.
This function does not seem to be quite right:
$affiliation->getLocalizedName() will not consider locale from the locale precedence,
and when a name value is found, we need to make a break (and not collect everything).
| } | ||
|
|
||
| /** | ||
| * Get the localized affiliations as a string. |
There was a problem hiding this comment.
localized affiliations names
There was a problem hiding this comment.
Hi @GaziYucel, thanks a lot for the good work and improvements!
I left a few comments.
I believe, that having the ROR object in an affiliation would make it easier to deal with names in different locales, e.g. we will not need to worry about setting ROR default name to the affiliation name with the submission locale, and also getting getLocalizedAffiliations... should be easier. I am only not sure how that would work with the ui-library... Let me know what do you think...
| @@ -182,7 +182,7 @@ public function initData() | |||
| $data = [ | |||
| 'givenName' => $author->getGivenName(null), // Localized | |||
| 'familyName' => $author->getFamilyName(null), // Localized | |||
| 'affiliation' => $author->getAffiliation(null), // Localized | |||
There was a problem hiding this comment.
Hmmm... This file is used in the quickSubmit plugin. I do not think we can use the new affiliationField UI component there :-( I/we need to think how to solve it best...
| @@ -225,7 +225,7 @@ public function execute(...$functionParams) | |||
| $author->setGivenName(array_map('trim', $this->getData('givenName')), null); | |||
| $author->setFamilyName($this->getData('familyName'), null); | |||
| $author->setPreferredPublicName($this->getData('preferredPublicName'), null); | |||
| $author->setAffiliation($this->getData('affiliation'), null); // localized | |||
There was a problem hiding this comment.
Note for myself: For this file as well. It is only used in quickSubmit plugin and we need to see how to solve the new affiliations there for 3.5
Related issues:
Related pull requests: