Add Crossref citations badge #3

bondjimbond · 2017-08-08T16:06:55Z

JIRA Ticket: (https://jira.duraspace.org/browse/ISLANDORA-2038)

What does this Pull Request do?

Adds a new Islandora Badges submodule, which generates citation counts via the Crossref REST API. This is a public API, without authentication, which would allow any Islandora site to get citation counts if they don't have access to Scopus or Web of Science credentials.

What's new?

A new badge titled "Islandora Crossref Citations". Displays citation counts for objects with DOIs, using the public-access Crossref REST API.

How should this be tested?

Enable the module and the block
Add a few objects with and without valid DOIs
View the object, check the results

Interested parties

@Islandora/7-x-1-x-committers @bryjbrown @mjordan

DiegoPino · 2017-08-08T16:18:29Z

@bondjimbond there is one DCS warning, an extra space in admin.form.inc

bondjimbond · 2017-08-08T17:06:50Z

@DiegoPino I'm pretty impressed with myself that Travis only found one problem this time.

DiegoPino

Some logic and minor doc concerns

DiegoPino · 2017-08-08T17:14:06Z

modules/islandora_crossref_citations/islandora_crossref_citations.module

+            $url = "https://api.crossref.org/works/";
+            $request_url = $url . $doi;
+            // Make the request and get results!
+            if (!isset($result_json->error)) {


This seems to be wrong. How can $result_json->error exist if it is defined later on?

Ack, you're right. That was in islandora_oadoi, which this is based on. Should be removed there too. Testing on a DOI with no results doesn't create any problems, so it should be fine to remove it. I'll commit a fix.

Would it be bad form to add the OADOI fix to this PR?

Yes. OADOI fix should go in a separate one, if it is a code task then it could be ok, but its more like a bug fix right?

DiegoPino · 2017-08-08T17:14:48Z

modules/islandora_crossref_citations/islandora_crossref_citations.module

+            // Make the request and get results!
+            if (!isset($result_json->error)) {
+              $result_json = drupal_http_request($request_url);
+              $result_array = json_decode($result_json->data, TRUE);


If the request fails, does ->data exists?

Nope, but if request fails, we don't get a badge or an error, which is what we want.

DiegoPino · 2017-08-08T17:15:48Z

modules/islandora_crossref_citations/islandora_crossref_citations.module

+      $badge = $block_text . ': ' . $result_citation_count;
+    }
+    else {
+      $badge = '<img src="https://img.shields.io/badge/' . $block_text . '-' . $result_citation_count . '-blue.svg?style=flat" alt="Number of citations via Crossref: ' . $result_citation_count . '">';


is $block_text being escaped to avoid attacks on img.shields.io?

No. How would I go about escaping it? (Though I expect they're already escaping input, since the general public can send whatever they want to the service.)

You need to make that one safe/clean manually. See https://www.drupal.org/docs/7/security/writing-secure-code-0/handle-text-in-a-secure-fashion. I think simply storing an user input from a form into a drupal var does not remove possible breaking arguments for img.shields.io, no cleanup is being made. Good practices there could be run the whole src content which is an url? through https://api.drupal.org/api/drupal/includes%21common.inc/function/check_url/7 but you could also sanitize/check on form validation/submit? where not sure, but somewhere for sure. Also since check_plain() and check_url() could rewrite your values, but users could be not aware, it would be good to maybe store this already cleaned up.. at least the form will load with the cleaned default and smart users will notice the difference? Not sure here, what do other @Islandora/7-x-1-x-committers think here?

check_plain() seems to be helpful. Tried it with some HTML in my variable, with and without... check_plain works.

Do we need check_url as well, or is that enough?

I guess it should be enough. Lastly (sorry, i'm worst than travis!), what about putting the alt="Number of citations via Crossref: ' . $result_citation_count . into a t() function with replacements for international users?

No problem. Done, I think?

DiegoPino · 2017-08-08T17:16:53Z

modules/islandora_crossref_citations/README.md

+This module requires the following modules/libraries:
+
+* [Islandora](https://github.com/islandora/islandora)
+* [Islandora Scholar](https://github.com/Islandora/islandora_scholar)


If Scholar is a dependency, why is it not part of the .info file? Is that so in every other submodule?

Scholar dependency should be removed. Adding to commit list.

DiegoPino · 2017-08-08T17:18:28Z

modules/islandora_crossref_citations/README.md

+
+* [Islandora](https://github.com/islandora/islandora)
+* [Islandora Scholar](https://github.com/Islandora/islandora_scholar)
+* [Islandora Badges](../../)


Since this is a submodule (in terms of publishing) but is just a module for Drupal's eyes, it seems to me that using relative paths in the documentation is very context dependent. .md files should be readable outside the github context.

DiegoPino · 2017-08-08T17:19:21Z

modules/islandora_crossref_citations/README.md

+
+Block text: Defines the text used in the citation count block. Defaults to "Citations via Crossref".
+
+Badge type: You can choose between plain text or an automatically-generated image. Plain text is offered for custom styling.


How is badge image generating website used? io? could that be added in the readme?

DiegoPino · 2017-08-08T18:30:23Z

@bondjimbond https://api.drupal.org/api/drupal/includes%21common.inc/function/drupal_http_request/7.x
I feel you should use the response codes there to decide if further processing is needed or you can throw some message or exception. There are some constants for time outs, etc, but probably sticking with error | no error should be enough. Just a suggestion.

bondjimbond · 2017-08-08T18:35:46Z

@DiegoPino I think that's what the if statement in your first review comment was attempting to get at - it was just added one line too early. I've adjusted that with the last commit, seems to be working.

DiegoPino

Looks fine now! 24 hrs and we should be clear

bondjimbond · 2017-08-10T14:23:24Z

@DiegoPino Nobody seems to have any concerns... Are we ready to merge? 😀

DiegoPino · 2017-08-10T14:27:45Z

@bondjimbond give me a while (need to talk to @manez). It just came to my attention that even if this module is a "submodule" that fact is really just a convention, and it is in strict, not so strict, definition (see the naming of the functions) this is a new module. So we need to probably discuss if we should consider it as such, or as part of badges.. i know it sounds lame, but trust me, better to have this discussion now than later one.
Edit: what i mean here is that: if this module (and every consequent ) submodule is really a independent module and if so, it could need "new module" process we have at islandora. If not, and submodules are fine as part of the parent, then all Ok. But it is something we should discuss.

manez · 2017-08-10T14:34:33Z

@DiegoPino @bondjimbond Let's put this on the agenda for the Committers Call today. It's only a few hours away, so it wont hold things up too much,

manez · 2017-08-10T18:36:06Z

Just for the record, the Committers discussed it and determined this one is fine, but there's a larger issue about whether submodules should have license files of their own, which we're sending up to the Board of Directors as a legal question that's beyond the scope of Committers.

This should not hold up merging this PR, but should hopefully lead to a policy we can follow by the next release.

Add Crossref citations badge

8d15247

Fix whitespace in admin form

c264303

DiegoPino suggested changes Aug 8, 2017

View reviewed changes

Fixing Diego's suggestions

73be7a6

Add error handling to drupal_http_request

e6d558c

Brandon Weigel added 2 commits August 8, 2017 19:22

Force plaintext in block_text variable

b62bde7

Add translatable alt text to badge image

da0fb35

DiegoPino approved these changes Aug 8, 2017

View reviewed changes

DiegoPino merged commit a271f86 into Islandora:7.x Aug 10, 2017

bondjimbond deleted the crossref-citations branch August 10, 2017 18:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Crossref citations badge #3

Add Crossref citations badge #3

bondjimbond commented Aug 8, 2017

DiegoPino commented Aug 8, 2017

bondjimbond commented Aug 8, 2017

DiegoPino left a comment

DiegoPino Aug 8, 2017

bondjimbond Aug 8, 2017

bondjimbond Aug 8, 2017

DiegoPino Aug 8, 2017

DiegoPino Aug 8, 2017

bondjimbond Aug 8, 2017

DiegoPino Aug 8, 2017

bondjimbond Aug 8, 2017

DiegoPino Aug 8, 2017

bondjimbond Aug 8, 2017

DiegoPino Aug 8, 2017

bondjimbond Aug 8, 2017

DiegoPino Aug 8, 2017

bondjimbond Aug 8, 2017

DiegoPino Aug 8, 2017

DiegoPino Aug 8, 2017

DiegoPino commented Aug 8, 2017

bondjimbond commented Aug 8, 2017

DiegoPino left a comment

bondjimbond commented Aug 10, 2017

DiegoPino commented Aug 10, 2017 •

edited

Loading

manez commented Aug 10, 2017

manez commented Aug 10, 2017


		Block text: Defines the text used in the citation count block. Defaults to "Citations via Crossref".

		Badge type: You can choose between plain text or an automatically-generated image. Plain text is offered for custom styling.

Add Crossref citations badge #3

Add Crossref citations badge #3

Conversation

bondjimbond commented Aug 8, 2017

What does this Pull Request do?

What's new?

How should this be tested?

Interested parties

DiegoPino commented Aug 8, 2017

bondjimbond commented Aug 8, 2017

DiegoPino 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

DiegoPino commented Aug 8, 2017

bondjimbond commented Aug 8, 2017

DiegoPino left a comment

Choose a reason for hiding this comment

bondjimbond commented Aug 10, 2017

DiegoPino commented Aug 10, 2017 • edited Loading

manez commented Aug 10, 2017

manez commented Aug 10, 2017

DiegoPino commented Aug 10, 2017 •

edited

Loading