pkp/pkp-lib/issues/6201 Adapt to support OMP #79

NateWr · 2021-06-01T11:36:30Z

No description provided.

NateWr

This looks really great @nongenti, thanks for the contribution! I've left some comments here. Some of it is just code syntax/style. But there are a few things we'll want to address to reduce code duplication between OJS/OMP, and keep it easy to maintain going forward.

I'm happy to talk through any questions you have about my comments and look forward to getting this merge! 👍

NateWr · 2021-06-01T11:36:55Z

CitationStyleLanguagePlugin.inc.php

+			$publication = $submission->getCurrentPublication();
+			$issue = NULL;
+			if (NULL === $publication) {
+				return FALSE;


Use lowercase null, true and false here and throughout.

NateWr · 2021-06-01T11:37:48Z

CitationStyleLanguagePlugin.inc.php

+				HookRegistry::register('Templates::Catalog::Book::Details', array($this, 'displayCitationMonograph'));
+			} else {
+				HookRegistry::register('ArticleHandler::view', array($this, 'getTemplateData'));
+			}


You don't need the if condition here. You can register all hooks all the time since they will only fire in each application.

NateWr · 2021-06-01T11:39:50Z

CitationStyleLanguagePlugin.inc.php

+			$languages = $submissionLanguageDao->getLanguages($publication->getId(), array(AppLocale::getLocale()));
+			if (array_key_exists(AppLocale::getLocale(), $languages)) {
+				$citationData->languages = $languages[AppLocale::getLocale()];
+			}


I think we'd like to see keywords and languages supported by OJS too, if these are supported by the relevant citation formats.

NateWr · 2021-06-01T11:42:51Z

CitationStyleLanguagePlugin.inc.php

+									$citationData->translator = array();
+								}
+								$citationData->translator[] = $currentAuthor;
+								break;


This won't work in many cases because the user groups are arbitrary. A journal can create their own user groups for these designations, or edit the existing ones. For this to work reliably, I think you will need to add configuration settings to the plugin which allows someone to select which user groups should be identified as editors, authors or translators.

If this is relevant to OJS, I'd like to see this supported for articles too.

Thank you @NateWr for review this! I have changed all the things above.
Like you recommend, I've added a configuration for the roles, now. I think translators are relevant to OJS, editors not. The settings-template discerns between OMP and OJS.

For chapter citation in OMP we need one role more, a chapter author. Should I include that mapping also here or in the settings of the chapter-landing-page-plugin. I think for useability its better here. I would show it, of course, only if the chapter-landing-page-plugin is enabled. If you want, I can wait for that decision, till you have seen the chapter-landing-page-plugin. I hope, that I will finish it in the next two or three weeks.

Yeah I think put that into this plugin, if the other plugin is enabled. Let me know once that's done and I'll do another review, thanks!

Well, it's done. You can review again. Thank you!

NateWr · 2021-06-01T11:45:17Z

CitationStyleLanguagePlugin.inc.php

+		if ($publication->getData('pub-id::doi')) {
+			$citationData->DOI = htmlspecialchars($publication->getData('pub-id::doi'));
+		}
+


This information should be added by the pub ID plugin itself. It looks like it's added by OJS's DOI plugin but not OMP's. You can see the OJS example here.

Well, I've removed it. Maybe I will have time next week to add this also to OMP's DOI plugin.

NateWr · 2021-06-01T11:47:18Z

CitationStyleLanguagePlugin.inc.php

-			$issue = $issueDao->getById($article->getCurrentPublication()->getData('issueId'));
+	public function downloadCitation($request, $submission, $citationStyle = 'ris', $issue = null) {
+		if ( !self::isApplicationOmp() )
+		{


Move this bracket up to the previous line.

I've changed it.

CitationStyleLanguagePlugin.inc.php

NateWr · 2021-06-01T11:55:36Z

templates/citationblock.tpl

+			</div>
+		</section>
+	</div>
+{/if}


We're going to end up with this template code (and the CSS code) duplicated across OJS/OMP. But each application will have it stored in a different place. Let's try to bring these into alignment if possible.

I like that your template/css code is stored in the plugin. Let's see if we can duplicate this in OJS as well. One way to do that is to include this template from the article_details.tpl file in OJS (see here). We can do that with something like:

{if $citation} {include file=$howToCiteTemplate}  {/if}

That way both will be loading from the plugin. We can then remove the CSS styles from OJS's default theme and rely on the ones in this plugin. We should do the same with the JS code that makes it go (this is in the default theme right now).

That's fine with me! Do you change the article_details.tpl? I will do it the same way in the template for the OMP chapter landing pages, what I'm also working on. Are you sure, that there is JS code for this plugin in the theme? I thought it's all in the plugin. (see here)

You're right! I thought that this used the dropdown JavaScript code from the default theme but it looks like it doesn't.

NateWr · 2021-06-01T11:58:07Z

css/citationStyleLanguagePlugin.css

+  box-shadow: none;
+  padding: 0 1em;
+  width: 100%;
+  font-family: "Noto Sans", -apple-system, BlinkMacSystemFont, "Segoe UI", "Roboto", "Oxygen-Sans", "Ubuntu", "Cantarell", "Helvetica Neue", sans-serif;


I think we need to be a little more careful about how we're interacting with non-default themes. This would override the theme's existing font selection. I can think of a couple ways around this:

The CSL plugin provides general layout styles, but leaves the font family, style, size and colors to the theme.

We try to load these styles into the compiled LESS stylesheet for the theme, when requested.

I'm happy to talk through these options with you.

I would prefer option one, because then the syle for the plugin is in the plugin. I've made some changes in the css file, so there shouldn't be problems with non-default themes any more. Only font size for the names of the citation styles in the dropdown and neccessary instructions for the arrow by FontAwesome are set by the plugin, now. Color instructions are also only set for some borders of dropdown.

NateWr · 2021-06-01T11:59:23Z

pages/CitationStyleLanguageHandler.inc.php

+			if ($this->returnJson) {
+				return new JSONMessage(false);
+			}
+			exit;


Let's use the 404 handler instead of exiting early. We can do that with $request->getDispatcher()->handle404();.

I can see that you''re just duplicating the exit below. But I'd prefer that we return 404 if we can.

I've changed it.

…eformat code

NateWr

Good, I think we're getting close! A few more comments but nothing major.

There is one more chunk of work that we'll need to figure out, though. Right now this is prepped against the stable-3_3_0 branch. We'll also need to port this over to the main branch. There have been some pretty big changes to the main branch because we're doing some new code formatting, so it won't be possible to cherry-pick the work into the main branch.

Once you've addressed the new PR comments, can you prepare a separate PR for your changes to the main branch?

In that PR, we'll also want to sort out the template/CSS stuff, so we'll need to adapt the default theme in OJS as well. That will require moving the template code out of article_details.tpl as well as the CSS code out of article_details.less, so that OJS is using the HTML/CSS from the plugin instead of the default theme.

NateWr · 2021-06-08T08:12:58Z

templates/citationblock.tpl

+			</div>
+		</section>
+	</div>
+{/if}


You're right! I thought that this used the dropdown JavaScript code from the default theme but it looks like it doesn't.

NateWr · 2021-06-08T08:15:20Z

CitationStyleLanguagePlugin.inc.php

+			$issue = null;
+			if (null === $publication) {
+				return false;
+			}


It shouldn't be possible for no $publication to exist here. The book view page would not work. Did you run into this yourself? If not, you can remove this check.

NateWr · 2021-06-08T08:21:21Z

CitationStyleLanguagePlugin.inc.php

+							}
+							$citationData->translator[] = $currentAuthor;
+							break;
+						case $this->getAuthorGroup($context->getId()):


Each time you call one of the methods to get the editor/translator/author group, you'll make a separate request to the database. I think with a switch statement that means it could run through the checks 3 times per author.

Can you get the editor/translator/author groups before the switch statement, so that each case line doesn't make a new run to the database?

NateWr · 2021-06-08T08:22:39Z

CitationStyleLanguagePlugin.inc.php

 		// TODO this is likely going to cause an error in a citation some day,
 		// but is necessary to get the .ris downloadable format working. The
 		// CSL language doesn't seem to offer a way to indicate a line break.
 		// See: https://github.com/citation-style-language/styles/issues/2831
 		$citation = str_replace('\n', "\n", $citation);

-        $encodedFilename = urlencode(substr($article->getLocalizedTitle(), 0, 60)) . '.' . $styleConfig['fileExtension'];
+        $encodedFilename = urlencode(substr(($publication ? $publication->getLocalizedTitle() : ''), 0, 60)) . '.' . $styleConfig['fileExtension'];


Convert these spaces to tabs.

NateWr · 2021-06-08T08:24:33Z

CitationStyleLanguageSettingsForm.inc.php

+			$context = $request->getContext();
+			$contextId = $context ? $context->getId() : 0;
+			$chapterPlugin = PluginRegistry::getPlugin('generic', 'chapterfrontendpageplugin');
+			$this->isChapterFrontendPagePluginEnabled = NULL !== $chapterPlugin && $chapterPlugin->getEnabled($contextId);


I'm trying to remember, but I think you might run into timing issues here, because I think the order that plugins are registered can change. So you may run into cases where the CSL plugin is running its constructor before the chapter frontend plugin has been registered.

NateWr · 2021-06-08T08:31:11Z

locale/en_US/locale.po

+msgstr "Author and Translator"
+
+msgid "plugins.generic.citationStyleLanguage.settings.citationUserGroupsDescription"
+msgstr "In some cases beside authors there also exists translators. Please choose a role for authors and translators."


In some citation styles, contributors must be identified differently depending on their role. Please identify the appropriate roles.

NateWr · 2021-06-08T08:32:01Z

locale/en_US/locale.po

+msgstr "Translator:"
+
+msgid "plugins.generic.citationStyleLanguage.settings.citationChooseChapterAuthor"
+msgstr "Chapter Author:"


Remove the : from each of these strings and make each one plural:

Authors

Editors

Translators

Chapter Authors

NateWr · 2021-06-08T08:33:58Z

locale/en_US/locale.po

+msgstr "Please choose a role for translators"
+
+msgid "plugins.generic.citationStyleLanguage.settings.citationOptionChooseChapterAuthor"
+msgstr "Please choose a role for chapter authors"


It is possible for for multiple user groups to be configured for each of these roles. I think we're going to need to adjust this to allow journals/presses to select multiple user groups for each role. Once changed, you can use the following text for each:

Select the roles that should be identified as authors.

NateWr · 2021-06-08T08:37:19Z

templates/settings.tpl

@@ -37,6 +43,69 @@
 				{fbvElement type="checkbox" id="enabledCitationDownloads[]" value=$id checked=in_array($id, $enabledDownloads) label=$style translate=false}
 			{/foreach}
 		{/fbvFormSection}
+		{fbvFormSection list=true title="{if $isApplicationOmp}plugins.generic.citationStyleLanguage.settings.citationUserGroups.omp{else}plugins.generic.citationStyleLanguage.settings.citationUserGroups{/if}"}
+			<p>{if $isApplicationOmp}{translate key="plugins.generic.citationStyleLanguage.settings.citationUserGroupsDescription.omp"}{else}{translate key="plugins.generic.citationStyleLanguage.settings.citationUserGroupsDescription"}{/if}</p>
+			<div class="pkp_helpers_quarter inline">


We're trying not to use these helper classes for half/quarter width. Just put each field on its own line, with the label above it. I'd recommend the following structure:

{fbvFormArea ...} {fbvFormSection label="..." ...} <select>...</select> {/fbvFormSection} {fbvFormSection label="..." ...} <select>...</select> {/fbvFormSection} ... {/fbvFormArea}

CitationStyleLanguageSettingsForm.inc.php

… roles.

nongenti · 2021-06-09T09:11:36Z

I've finished these changes.

Can you tell me, whether I've a chance to get the main branch code running with OMP 3.3.0.3/OJS 3.2.1.4? Because that's the most updated development system I can use. If it's possible, I will try to port this to the main branch.
Is anywhere a coding convention for the new code format or exists a settings-file for phpstorm for that?

NateWr · 2021-06-09T14:29:22Z

Thanks @nongenti! I think you should be able to run the main branch off of a database from 3.3. I'm not sure, but I can't think of any significant database changes yet.

nongenti · 2021-06-14T11:50:58Z

Well, I will port this to the main branch, but it will take a little longer.

NateWr · 2021-06-15T07:46:10Z

That's great, thanks @nongenti, let me know if you have any questions about it.

nongenti · 2021-11-23T17:19:54Z

Hi @NateWr, I've create a new pull request for the main branch now.

nongenti added 3 commits May 31, 2021 11:26

Add OMP 3.3 compatibility.

b237fd7

Fix: OMP compatibility

1d7f5fe

Change readme

b756515

NateWr commented Jun 1, 2021

View reviewed changes

nongenti added 2 commits June 2, 2021 16:48

Add select a role for author, editor and translator to settings and r…

5b68c41

…eformat code

Add select a role for chapter author to settings

9bcab49

NateWr commented Jun 8, 2021

View reviewed changes

Reformat code, change wording and add select multiple user groups for…

ed120eb

… roles.

nongenti mentioned this pull request Nov 23, 2021

pkp/pkp-lib/issues/6201 Adapt to support OMP #84

Closed

NateWr closed this Apr 25, 2022

nongenti mentioned this pull request Apr 26, 2022

Port Citation Style Language plugin to OMP pkp/pkp-lib#6201

Closed

NateWr reopened this Apr 26, 2022

asmecher changed the title ~~Adapt to support OMP~~ pkp/pkp-lib/issues/6201 Adapt to support OMP May 17, 2022

pkp/pkp-lib/issues/6201 Adapt to support OMP #79

Are you sure you want to change the base?

pkp/pkp-lib/issues/6201 Adapt to support OMP #79

Conversation

NateWr commented Jun 1, 2021

NateWr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NateWr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nongenti commented Jun 9, 2021

NateWr commented Jun 9, 2021

nongenti commented Jun 14, 2021

NateWr commented Jun 15, 2021

nongenti commented Nov 23, 2021