Implement Piwik plugin for OJS 3.0

[kaschioudi]

@asmecher : here's an initial work based on google analytics plugin : https://github.com/kaschioudi/piwik

[kaschioudi]

@bozana

[bozana]

@kaschioudi, Great work!

I have just a few comments: version.xml: -- maybe to change the date

PiwikSettingsFor: -- readInputData: both variable can be in one array and one line -- execute: piwikSiteId was "int" earlier, is "string" correct now? -- just double checking :-) -- the first two FormValidator are not necessary if the FVB form fields are defined as required (attribute required=true), because then the AjaxFormHandler takes care about that. Then also the two locale keys are not needed any more.

settingsForm.tpl: -- the file path in the file comment doesn't contain "templates" folder -- the form fields should be defined as required, so that AjaxFormHandler can take care of that. I think this is a better way to deal with such simple required fields. Then also the inPlaceNotification is not necessary any more, I believe. -- instead of or additionally to the "name" attribute, the "id" attribute should be used for FBV form fields, else there will be PHP Error/Notice, if the id is missing, I think. And the name is then generated automatically, I think.

locales: -- what about other locales? Although the OJS 3.0 is not translated in many languages, we take them.

I am not familiar with the Piwik code that should be inserted, thus I assume it is correct. I also haven't tested it. Did/could you test it somehow? I'll try to test it just locally... Thanks a lot!

[kaschioudi]

Thank you very much @bozana for you review. When you get a chance, please take a look at these commits (https://github.com/kaschioudi/piwik/commit/6f673e78a170955ae9c5b8785e92b28f4dcd71cd & https://github.com/kaschioudi/piwik/commit/dfb4c025d2fe0cdd5e0a92f77f1d26913377e2c3) and let me know if I have correctly applied your suggestions.

-- execute: piwikSiteId was "int" earlier, is "string" correct now? -- just double checking :-)

I wasn't sure whether piwikSiteId is int or string. I am under the impression that it's integer, but I put string for now. I will validate once I have a running piwik install.

-- what about other locales? Although the OJS 3.0 is not translated in many languages, we take them.

I was not sure what the procedure is exactly. I have copied French and English because I was able to modify / adapt the content for piwik. From my perspective, it requires more than simple replacements of Google Analytics with Piwik. @asmecher could you please advise here? thanks.

I am not familiar with the Piwik code that should be inserted, thus I assume it is correct.

Javascript integration code came from https://developer.piwik.org/guides/tracking-javascript-guide

[bozana]

Hi @kaschioudi, just a note for the locales -- you can get them from the ojs-stable-2_4_8 branch. Those keys, that you changed should be deleted from other locales. Of course also those that you do not need any more i.e. that you removed. I'll take a look at the commit soon...

[bozana]

@kaschioudi, I am so sorry, I think I said a wrong thing :-((( I've just tested it and apparently there have to be both elements: the FormValidator and required=true :-O I don't know why, because the error message displayed is always the same i.e. not the one defined in the FormValidator. Sorry, sorry, sorry :-( @asmecher, is this supposed to be so?

@kaschioudi, I found that the old Piwik plugin also sets the DocumentTitle, s. https://github.com/pkp/ojs/blob/ojs-stable-2_4_8/plugins/generic/piwik/PiwikPlugin.inc.php#L179. I don't know exactly why -- maybe it is easier in the case of a multi journal installation -- but this can maybe be checked once we have a Piwik installation to test it with. Else, everything fine :-)

[asmecher]

We're doing validation both on the front end (Javascript-based) and in the back-end (PHP based). The required="true" affects the Javascript validation, and when those requirements aren't met, the form doesn't even get submitted before the warning is shown.

We don't have a strict 1:1 mapping between these validations because the PHP-side validation can be more stringent -- e.g. checking to make sure that journal paths are unique, which requires going to the database.

[bozana]

@asmecher, but in order for the message "This field is required" to be displayed when you leave such a required field, you have to define both i.e. the field should be required and there have to be a FormValidator for that field, right? Is then in simple cases just required attribute for the form field enough?

[asmecher]

Both should be necessary, I think -- and the PHP side is definitely required, as e.g. bad actors can bypass front-end based checks.

[kaschioudi]

no worries @bozana. I have put back those validation lines. I didn't know there was an existing piwik module for ojs2. I have imported all the locales and got rid not needed entries. I have also modified the setting form to display more instructions. (see https://github.com/kaschioudi/piwik/commit/2af735be87c9f74bb4053335be00f25762a1e089)

@asmecher: ojs2 piwik module's copyright has

* Copyright (c) 2013-2016 Simon Fraser University Library

while I have used

* Copyright (c) 2014-2016 Simon Fraser University Library (which I copied from ojs3 Google analytics plugin).

Which year should I use? 2013 or 2014 ?

[kaschioudi]

@bozana: DocumentTitle tracking added: https://github.com/kaschioudi/piwik/commit/b14213e1fa6f87e41fc0316c07547aa01f1f2525

[asmecher]

@kaschioudi, I don't think the start dates are especially important. I'd suggest going with the earlier dates if in doubt (e.g. if you're combining two sources).

[bozana]

@kaschioudi, thanks a lot! I think now everything is OK. I see you changed these locale keys: plugins.generic.piwik.description plugins.generic.piwik.manager.settings.description plugins.generic.piwik.manager.settings.piwikSiteId plugins.generic.piwik.manager.settings.piwikUrl Maybe to remove them then from other locales?

[kaschioudi]

Okay @asmecher. I will use 2013 for all files. thanks

@bozana : I did not change those locale keys. I have reused everything from piwik module for ojs2. I got rid of plugins.generic.piwik.manager.settings and plugins.generic.piwik.manager.piwikSettings entries in all locales because I was not using them.

In case you are referring to https://github.com/kaschioudi/piwik/commit/671387c009b7cdbb3a8dd70db1e3de3fc996c266#diff-39ad57a0a799a092b476f7b3d029e60dL18 and https://github.com/kaschioudi/piwik/commit/671387c009b7cdbb3a8dd70db1e3de3fc996c266#diff-4c30bba1d450b5fbbb1d1b6fe5cf2456L21, please note that I was actually reverting my attempt to adapt Google analytics plugin sentences to piwik.

[bozana]

Ah sorry @kaschioudi, yes I meant them :-( Everything fine then :-))) Thanks a lot!!!

[bozana]

@asmecher, I think the plugin is finished. Should the plugin be a PKP repository and be integrated as such/submodule? Or what would be the next steps?

[asmecher]

I think it would just take a little bit of work to modify this so that it would work also with OMP -- so I'd suggest trying that, keeping it as a separate repository, and adding the plugin as a submodule to both OJS and OMP repos.

[asmecher]

@kaschioudi, I think this is still waiting on some work from you, correct?

[kaschioudi]

@asmecher : I believe that the only thing left was to check piwikSiteId type.. which is integer instead of string. I just pushed a fix (https://github.com/kaschioudi/piwik/commit/e032e8d166078afd2ce74b3497e8202a32e8d7d8). I believe this is good to go.

[asmecher]

It looks like it still needs some adapting for OMP, though. I think (not having looked at it) that it should be pretty applicable to both apps if you use some generic terminology (e.g. contextId rather than journalId -- mind taking a look?

[asmecher]

Done! https://github.com/pkp/piwik