CFM-121_ORCID_Source_Plugin #311
Conversation
Ioannis
force-pushed
the
CFM-121_ORCID_Source_Plugin
branch
from
45fcd1a
to
01a9d39
Compare
Dispatch... dispatch... Dispatch and Hydrate external identity sync... external identity sync... sync external identity... ORCID Source search... Same EIS different Person ID
Ioannis
force-pushed
the
CFM-121_ORCID_Source_Plugin
branch
from
01a9d39
to
54033ad
Compare
There was a problem hiding this comment.
There's a comment in the documentation that "The ORCID Identifier is now called the Source Key."
The ORCiD should show up as an Identifier of type orcid. It looks like resultToEntityData does this?
|
|
||
| protected function manageEISRecord( | ||
| Pipeline $pipeline, | ||
| ExternalIdentitySource $eis, | ||
| string $sourceKey, | ||
| ?string $sourceRecord, |
There was a problem hiding this comment.
I rolled back this change in a6c77a2898 since it was breaking pipeline updates ($personId is intentionally not passed through on update requests, so they were all failing), but also because manageEISRecord is only supposed to work within the EIS Record context, it's not supposed to know about the Person record.
Let's discuss what this test is supposed to be doing, there may be a better place for it.
There was a problem hiding this comment.
Should we commit this file? It doesn't actually apply...
There was a problem hiding this comment.
@benno other plugins have this files too. This is why i left it in there. Do we remove them from everywhere?
There was a problem hiding this comment.
Similarly, do we need to commit this file? The name is wrong, the license is wrong, and the autoload paths should have been addressed by the main composer.json mods above.
There was a problem hiding this comment.
@benno the same as above
| // XXX We could maybe move this into StandardEnrollerController with a flag like | ||
| // $this->alwaysAuthDispatch(true); |
There was a problem hiding this comment.
I've added an item to CFM-47, we should review this for the next plugin since there are now three that do something similar
| use App\Lib\Enum\StandardEnum; | ||
|
|
||
| class OrcidSourceScopeEnum extends StandardEnum { | ||
| const DEFAULT_SCOPE = '/authenticate'; |
There was a problem hiding this comment.
Do we really need an enum if there's only one value? Can we just define the CONST instead?
There was a problem hiding this comment.
This is used in more than one files. This way we can include it without having to duplicate the code
| $role = [ | ||
| // We only support one role per record | ||
| 'role_key' => '1', | ||
| 'affiliation' => $this->DefaultAffiliationTypes->getTypeLabel($OrcidSource->default_affiliation_type_id) | ||
| ]; |
There was a problem hiding this comment.
Does it make sense for OrcidSource to assert an External Identity Role? (It should be possible for an EIS to return a record without one.)
If so, does the affiliation type need to be configurable? It's not clear what an affiliation for an ORCiD record should be... are we somehow propagating information about the underlying ORCiD record? This probably needs some discussion.
No description provided.