defstat / pln

GNU General Public License v3.0
0 stars 8 forks source link

Integration with journal setup does not work #31

Closed asmecher closed 5 years ago

asmecher commented 5 years ago

The PLN plugin registers for a hook called Templates::Manager::Setup::JournalArchiving to augment journal settings with information on the PLN plugin. However, that hook no longer exists in OJS 3.1.2, so the intended text is never displayed and the hook callback etc. is currently dead code.

defstat commented 5 years ago

@asmecher @natewr I am also getting a http 500 error when trying the Distribution menu on OJS's master branch image

PHP Fatal error: Uncaught Error: Class 'PKP\components\forms\FieldArchivingPn' not found in \ojs-3-dev\pages\management\SettingsHandler.inc.php:94 Could that be related somehow?

defstat commented 5 years ago

I think that this was added in order for the status of the pln plugin to be displayed on the Journal's management pages. Has that been removed, or has it been moved at the Distribution page?

NateWr commented 5 years ago

Someone found and fixed this recently, maybe @ajnyga? Looks like the fix has not made it to master and I can't find it filed in an existing issue. Anyway, the error is here:

https://github.com/pkp/ojs/blob/master/pages/management/SettingsHandler.inc.php#L94

\PKP\components\forms\FieldArchivingPn should be \APP\components\forms\FieldArchivingPn.

defstat commented 5 years ago

@NateWr Should I also add use \PKP\components\forms\FieldOptions; before https://github.com/pkp/ojs/blob/master/classes/components/forms/FieldArchivingPn.inc.php#L17

Should the Archiving tab be displaying something?

NateWr commented 5 years ago

Should I also add use \PKP\components\forms\FieldOptions; before https://github.com/pkp/ojs/blob/master/classes/components/forms/FieldArchivingPn.inc.php#L17

No. The \APP\components... namespace will load from classes/components and the \PKP\components\... namespace will load from lib/pkp/classes/components.

The tab won't display something if the fatal error is occurring. But otherwise it should.

NateWr commented 5 years ago

Sorry I should have actually looked at your link. In the case you sent, yes, you need to add the namespace. (Sorry!)

defstat commented 5 years ago

@asmecher Regarding that: the use of that hook seems to serve the purpose of displaying the link that @NateWr had already incorporated in the implementation of the Settings->Distribution->Archiving tab form [see image below] image

The Template::Settings::distribution::archiving could be used, though, if we want to add a grid with the deposits bellow the LOCKSS and CLOCKSS tab. That requirement, I think, was part of the initial requirements regarding that page, when Mark had asked for some additions to the archiving page, like adding the ability to enable the PLN Plugin from that page - Should we consider adding that there?

asmecher commented 5 years ago

Ah, I hadn't realized the adaptation for the master branch had already taken place. We might need to support ojs-stable-3_1_2 as well, in which case this integration would need to be adapted (or we would need to decide it wasn't worth the trouble).

I do think it's worth adding to the LOCKSS and CLOCKSS tab.

asmecher commented 5 years ago

For the OJS 3.1.2 implementation, the PLN settings interface is coded into OJS itself (see https://github.com/pkp/pkp-lib/issues/2998) in a way that's essentially equivalent to what was coded into the plugin (and isn't currently working). For the 3.1.2 release, I suggest removing the redundant implementation in the plugin and letting the OJS setup interface handle it. In the future, it would be better to have this stuff live entirely inside the plugin, but that's an improvement better left to the master branch and its setup reimplementation.

asmecher commented 5 years ago

@defstat, can you review https://github.com/defstat/pln/pull/40 (see the above comment)?

asmecher commented 5 years ago

Thanks, @defstat -- closing!

mjordan commented 5 years ago

Anyone got a screenshot illustrating the Archiving page as it stands after this fix?

defstat commented 5 years ago

@mjordan For ojs 3.1.2 the screenshot image image It's as we had initially discussed.

For ojs 3.2 it's like it is shown at https://github.com/defstat/pln/issues/31#issuecomment-509721997.

In short, the hook that has been removed was not in use.

mjordan commented 5 years ago

Great, thanks!

mjordan commented 5 years ago

One suggestion for 3.2 - let the user know that table will be in the plugin settings, e.g. "View the plugin settings to accept the terms of use for the PKP PN and to see the status of your deposits."