-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
pkp/plagiarism#52 iThenticate API update and plugin update #57
pkp/plagiarism#52 iThenticate API update and plugin update #57
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some early comments -- I'll have a look at the schema extension issue next week.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more changes -- I'll review these with you shortly!
PlagiarismSettingsForm.inc.php
Outdated
); | ||
public function initData() { | ||
$this->_data = [ | ||
'ithenticateApiUrl' => $this->_plugin->getSetting($this->_contextId, 'ithenticateApiUrl'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(spaces in with tabs)
} | ||
|
||
public function handle() { | ||
error_log('handle webhook request'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably shouldn't leave this in the distribution version of the plugin, as it's debugging/dev code. But if we did, it should be prefixed with something to indicate more about what this is, e.g. "PKP Plagiarism plugin".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we will eventually implement some functionality through webhook but probably not for stable-3.3.0 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we initially planed not to use the webhook for 3.3.0-x
but we have to as without webhook, we can not initiate the similarity report generation process .
…d webhook registration on settings update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bunch more minor comments; I've mentioned a few of these before (mixed spacing/tabs and variants of EULA), so please double-check to make sure it's all cleaned up for the next round. Thanks!
IThenticate.inc.php
Outdated
class IThenticate | ||
{ | ||
/** | ||
* The base api url in the format of schems://host |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*scheme
IThenticate.inc.php
Outdated
protected $integrationVersion; | ||
|
||
/** | ||
* The EULA(end user license agreement) that user need to confrim before making request |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*confirm
IThenticate.inc.php
Outdated
} | ||
|
||
/** | ||
* Validate the service access by retrieving the the enabled feature |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the the
IThenticate.inc.php
Outdated
'Content-Type' => 'application/json' | ||
]), | ||
'json' => [ | ||
'owner' => $author->getId(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be using our internal IDs for owner
and submitter
? We use email addresses as unique account identifiers in OJS; maybe those would be more appropriate for users; we can't always count on having those for authors, though.
If this is something we can use for our own arbitrary purposes, it might be worth adding some kind of namespacing to avoid confusion -- e.g. author/54
and user/54
rather than just the numeric 54
. It'll be easy to forget where we're using User
s and where we're using Author
s and the IDs aren't comparable. If iThenticate ever does a comparison between them for e.g. permission purposes, it'll mistakenly conclude that they're the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
email addresses as unique account identifiers in OJS; maybe those would be more appropriate for users; we can't always count on having those for authors, though
That was my initial approach also but because an author may not have an email address and author/owner details is required, we could not use email
If this is something we can use for our own arbitrary purposes
Not actually . According to TAC developer doc, submission author/owner unique system identifier is required . The submitter information is optional but in that case owner/author must accept/confirm EULA . As in our case, the author/owner and submitter can be different and it's the submitter who confirms the EULA, we also need to pass it .
it might be worth adding some kind of namespacing to avoid confusion -- e.g.
author/54
anduser/54
I think that a good idea and will do it .
|
||
$response = Application::get()->getHttpClient()->request( | ||
'PUT', | ||
$this->getApiPath("submissions/{$submissionTacId}/original"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where we're building parameters into URL strings, we should probably be using rawurlencode
(here and elsewhere).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a bad idea but then rather than using the rawurlencode
directly we should let the Guzzel
handle it by returning an instance of \GuzzleHttp\Psr7\Uri
.
PlagiarismPlugin.inc.php
Outdated
public function retrieveApplicableEulaDetails($cache, $cacheId) { | ||
$context = Application::get()->getRequest()->getContext(); | ||
$ithenticate = $this->initIthenticate(...$this->getServiceAccess($context)); /** @var \IThenticate $ithenticate */ | ||
$eualDetails = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be eulaDetails
(here and below)
PlagiarismPlugin.inc.php
Outdated
* | ||
* @return void | ||
*/ | ||
protected function stampEualVersionToSubmittingUser($user, $version) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be Eula
, not Eual
.
PlagiarismSettingsForm.inc.php
Outdated
* @copydoc Form::validate() | ||
*/ | ||
public function validate($callHooks = true) { | ||
$this->addCheck( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checks should be added in the form constructor, not the call to validate
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we need to init the ithenticate service to test the given creds, using the getData
in constructor does not work and pass null . well fixed it using $request->getUserVar
and moved it to constructor .
locale/en_US/locale.po
Outdated
|
||
msgid "plugins.generic.plagiarism.manager.settings.areForced" | ||
msgstr "iThenticate settings were found in config.inc.php and the settings here will not be used." | ||
|
||
msgid "plugins.generic.plagiarism.errorMessage" | ||
msgstr "Upload of submission {$submissionId} to iThenticate failed with error: {$errorMessage}" | ||
|
||
msgid "plugins.generic.plagiarism.submission.checklist.eula" | ||
msgstr "Plagiarism EULA <a target=\"_blank\" href=\"{$localizedEulaUrl}\">link</a>" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be iThenticate EULA
, not Plagiarism EULA
, I think
locale/en_US/locale.po
Outdated
|
||
msgid "plugins.generic.plagiarism.manager.settings.serviceAccessInvalid" | ||
msgstr "" | ||
"The specified API URL/KEY is invalid and can not establish connection to IThenticate API service." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be iThenticate
, not IThenticate
@asmecher ready for next review round . forced to have the vital implementation in webhook handler as no other way . Also as we get submission id from ithenticate for each submission file , moved some the association to submission file setting . |
… along with user notification system on webhook handle
…as part of plugins settings
…wer launch functionality
Another issue to resolve: Try editing
...rather than sending the user to the installation form. |
@asmecher please the commit touhidurabir@6042142 . This resolve the above issue along with #49 but updated the behaviour a bit as follow
However I still think there is room for more improvement . With our current implementation, it is impossible to disable the plugin once enabled without globally disable it by setting For example , in a multi context based installation, may be the JM/Admin want to disable it for one specific context for some reason but no way to do it . My proposal is to remove the plugin based implementation of |
…wer launch url create process
…uired and validation passes
for #52 . The PR has following updates :
Following Issues get resolved by this PR