pkp / orcidProfile

A plugin to pull ORCID information into a PKP user profile
GNU General Public License v3.0
16 stars 50 forks source link

orcidProfile plugin option for email is unimplemented #112

Open ctgraham opened 5 years ago

ctgraham commented 5 years ago

Describe the bug The orcidProfile plugin settings form offers an option "Send e-mail to request ORCID authorization from article authors on publication of a new issue", but this setting is not referenced anywhere in the code, so mail is always sent on publication.

Reported: https://forum.pkp.sfu.ca/t/ojs-3-1-2-1-orcid-collect-letters-cannot-be-disabled/55206

To Reproduce Steps to reproduce the behavior:

  1. Go to orcidProfile plugin settings
  2. Check "Send e-mail to request ORCID authorization from article authors on publication of a new issue" and save settings
  3. Publish an issue with incomplete ORCID information for authors
  4. Mail will be sent regardless of the setting

What application are you using? OJS or OMP version 3.1.2

Additional information https://github.com/pkp/orcidProfile/blob/65b4c3beeb9759310189ea776ddc6f3e15feac5f/templates/settingsForm.tpl#L48-L50

vs.

https://github.com/pkp/orcidProfile/blob/65b4c3beeb9759310189ea776ddc6f3e15feac5f/OrcidProfilePlugin.inc.php#L705-L723

ctgraham commented 5 years ago

I haven't actually tested this patch, and I fear that it will not work as expected if register() is called without a context id.

We probably need something like:

        if ($contextId == null) {
            $contextId = $this->getCurrentContextId();
        }
asmecher commented 5 years ago

Merged and cherry-picked the additional commit; thanks, @ctgraham!

ohilbig01 commented 5 years ago

Hi @asmecher, I think there is an s missing in "sendMailToAuthorOnPublication"? sendMailToAuthorsOnPublication worked for me.

ctgraham commented 5 years ago

New PR filed. Thanks, @ohilbig01 !

asmecher commented 5 years ago

Thanks, @ohilbig01 and @ctgraham -- merged and cherry-picked to stable-3_1_2.

nils-stefan-weiher commented 5 years ago

Thanks @ohilbig01 and @ctgraham . That was a real big oversight from me.

ajnyga commented 4 years ago

Hi,

Noticed a potential problem with this same hook.

The setting says "Send e-mail to request ORCID authorization from article authors on publication of a new issue"

But the setting enables this hook here: https://github.com/pkp/orcidProfile/blob/master/OrcidProfilePlugin.inc.php#L75 EditorAction::recordDecision Which, to my knowledge, is not connected at all to publishing a new issue.

It is attached to this function https://github.com/pkp/orcidProfile/blob/master/OrcidProfilePlugin.inc.php#L716-L726 which does not check what kind of decision is done.

So the hook fires on every editorial decision I think. Meaning it could fire multiple times during the workflow. Explaining a recent forum post here: https://forum.pkp.sfu.ca/t/limit-orcid-plugin-notifications/57170

I already discussed this with @withanage and my suggestion is:

  1. Change the label for the setting to “Send e-mail to request ORCID authorization from article authors when the submission is moved to copy editing
  2. Edit the handleEditorAction function so that it will check the editor action and only fires when the action is "send to copyediting".

After that ORCIDs are collected and emails sent in three cases:

  1. A new user is registered
  2. Adding a new author to a submission (if the selection is checked in the form)
  3. Sending the submission to copy-editing (if the plugin setting is enabled)
ctgraham commented 4 years ago

@ajnyga, do you want to write up a PR for this fix, or are you looking for someone else to pick it up?

ajnyga commented 4 years ago

Pull requests for stable-3_1_2: https://github.com/pkp/orcidProfile/pull/82 master: https://github.com/pkp/orcidProfile/pull/83

ajnyga commented 4 years ago

@ctgraham

Updated pull requests here: stable-3_1_2: pkp/orcidProfile#82 master: pkp/orcidProfile#83

I am not changing the variable for now so that we can have this important fix implented asap. I will create a new issue about the variable name.

ctgraham commented 4 years ago

Sounds like a plan to me. Looks good here.

asmecher commented 2 years ago

@ajnyga, the PRs above were merged -- is this issue ready to be closed?