Settings for journal managers #31
Conversation
PKP Sprint initial review fixes
Add missing locale keys for required fields
Sprint dev
Sprint dev
add exception for bad passwords
Error messages and README
Error messages
Update README
|
Hi there. We do not understand why configuration file credentials supersede the Radek-settings credentials. At least, this is what we see from the code in this pull request, if we understand well. We would suggest the opposite. This is exactly what we tried to do with our fork. We have OJS 3.2 and we built a fork starting from plagiarism 1.0.3, and adding radek settings (thank you @radekgomola). You can find the fork here https://github.com/joelfan/plagiarism. As a matter of facts, our fork tries to get credentials from settings first. When not found, the plugin gets credentials from configuration file. By the way, we also added some magic in order to create a user in Ithenticate server, when non existent. We followed Radek instructions, they work like a stream also in OJS 3.2. If there is interest, we can port our fork to plagiarism 1.0.5 (which is for OJS 3.3), having then new bsobbe/ithenticate, user definition when not existent, user management, proxy support, php8 support. Thx. |
|
Hi, @joelfan . The configuration file supersedes the settings form in order to provide flexibility for installations where the administrator is different than the journal manager. For example, consider a hosting provider, where journal staff does not have access to edit the filesystem. This allows for the administrator to choose whether to force specific settings on behalf of the journal, or to allow the journal managers to configure their journal on their own. If the settings form overrides the config.inc.php file, then the administrator loses this option. We worked on this enhancement at the PKP Sprint in Helsinki, and at that event we looked at your changes to iThenticate with interest. We were unable to find the plagiarism plugin fork, so we were unable to fully evaluate your changes. We would like to get a better understanding of your proposed changes, but your fork marks the entire files as changed, rather than highlighting just the actual changes. See, for example: Could we work together to refactor your changes into clean pull requests, both on the plagiarism repo and on the iThenticate repo? This will help us understand the changes better and will help us know how to incorporate them. |
|
Thank you for your answer and of course we are happy to collaborate. As you might imagine, I am a collegue of bolelligallevi I am sorry for the fork that is not easily compared as a fork. Maybe the reason is what you wrote (line feed). We are not so familiar with github, if you want you can make a more genuine fork starting from joelfan/plagiarism or please tell me how I should do the fork from asmecher/plagiarism. Anyway, let me shortly summarize what we have done:
That's the logic at the moment (please consider that at the moment is for plagiarism 1.0.3 because we have ojs 3.2). We have also tested the so-called sharing logic, where everything is submitted by ithenticate administrator and then shared to the user(s) that are defined in the setting. It works, but this option was stopped because there is a limit in the number of folders by a single user in ithenticate, as you know. We are going anyway to upgrade our production OJS to 3.3 in September or October. Thank you again and of course keep us udated. |
PlagiarismPlugin.inc.php
Outdated
| $managersArray = $managers->toAssociativeArray(); | ||
| $allUserIds = array_keys($managersArray); | ||
| foreach ($allUserIds as $userId) { | ||
| $notificationManager->createTrivialNotification($userId, NOTIFICATION_TYPE_ERROR, array('contents' => __('plugins.generic.plagiarism.errorMessage', array('submissionId' => $submissionid, 'errorMessage' => $message)))); |
There was a problem hiding this comment.
I think I heard this raised already -- but I think a trivial notification probably isn't a good fit. It'll cause a toast notification that will go away, possibly before it can be read/digested, and there's no way to get it back. Maybe an email would be better as per e.g. the Paypal plugin? (The likeliest course of action will be that an editor will get an inscrutable error message and pass it on to an administrator of some kind, so maybe it should be going to the tech contact instead of all editors?)
There was a problem hiding this comment.
Agreed. We'd like to migrate these to Tasks instead of Trivial Notifications, but we didn't come up with an example of creating a Task in the Sprint. Error messages should be routed to Journal Managers or Administrators, depending on if the settings come from the form, or from config.inc.php. I could also imagine creating Tasks for Editors in the future as part of the review of the plagiarism check. I'd prefer to take care of this in an enhancement round after an initial merge.
There was a problem hiding this comment.
Propose to handle in a new PR.
PlagiarismSettingsForm.inc.php
Outdated
| * @copydoc Form::execute() | ||
| */ | ||
| function execute(...$functionArgs) { | ||
| $this->_plugin->updateSetting($this->_contextId, 'ithenticate_user', trim($this->getData('ithenticate_user'), "\"\';"), 'string'); |
There was a problem hiding this comment.
Is the trim call necessary? What if the password intentionally begins or ends with a "?
There was a problem hiding this comment.
@radekgomola asserted that this was strangely needed. I have a vague memory of encountering and removing this formulation in other plugins.
There was a problem hiding this comment.
Hej @radekgomola! Řekni mi co jsi našel.
|
@radekgomola , I have a a PR to your repository with a response to most of Alec's review requests. If you can review and merge to
|
Update README and settings description
|
@asmecher , your review comments are largely addressed, save the config.inc.php setting and trivial notification conversion, which I propose to submit as subsequent PRs. Can you take another look? |
|
Merged, thanks! |
No description provided.