Add shorten DOI field formatter #5276

dawidowoc · 2019-09-02T16:31:23Z

What this PR adds?

This PR is related to koppor#343.

I added a field formatter called Shorten DOI formatter which shortens DOI using http://shortdoi.org service. You run the formatter from Cleanup entries dialog by adding it to Run field formatter list.

It looks to work fine. It enables one way conversion from DOI to short DOI.

Implementation details:

I assumed that short DOI is still a legal DOI. That's why I adjusted the DOI class to work with short DOIs.

Feedback wanted

I I'd like to ask for your feedback about the code at this stage.

Next step

Add a more accessible UI - a toggle button next to the DOI field shortened/long version (This was suggested by @koppor)

This will require adding opposite conversion (from short DOI to DOI). As http://shortdoi.org servcie doesn't offer such a feature I see two approaches:

Store the original DOI during the conversion for further back conversion.
Drawback: You may not convert from short DOI to DOI if you haven't done the opposite conversion for at least one time.
Try to obtain DOI from short DOI using https://doi.org service as it is possible by catching the HTTP redirection

I find the 1st approach fine. The 2th approach is not really good as we are not using any public API and this can stop working in the future.

What do you think folks? :)

Important

If you would like to checkout this branch and test locally please use shortdoi-workaround branch which uses workaround by @r0light for issue #5245. On master branch there is an issue with accessing General tab in bibentry editor.

Change in CHANGELOG.md described
Tests created for changes
Manually tested changed features in running JabRef
Screenshots added in PR description (for bigger UI changes)
Ensured that the git commit message is a good one
Check documentation status (Issue created for outdated help page at help.jabref.org?)

Siedlerchr · 2019-09-02T18:52:17Z

src/main/java/org/jabref/logic/importer/util/ShortDOIServiceResponse.java

+import java.util.Objects;
+
+public class ShortDOIServiceResponse {
+    private String DOI;


make this variable lowercase (although it's an acronym)

Good call. Done.

Siedlerchr

codewise looks very good and also the fact that you added tests is really good!
Please take look at the travis checkstyle output., there seems to be some issues with the import order and missing braces around if clauses.
We also have code formatting templates for eclipse and intellij avaiable so that it allows you to easily fix such import issues

tobiasdiez

Thanks for your PR. The code looks already pretty good and I've only some minor suggestions.

Concerning the UI, in my opinion, the shorten doi feature shouldn't be placed that prominently (it's a nice-to-have feature but not one that compete with "get bibtex from doi" or "open doi url"). Thus, I would add it to the right-click menu (this is sadly blocked by #5254 at the moment). In this way, you also don't need to take care of the "unshorten doi" problem. @koppor what do you think?

tobiasdiez · 2019-09-03T09:32:18Z

src/main/java/org/jabref/logic/formatter/bibtexfields/ShortenDOIFormatter.java

+        DOI doi = null;
+
+        try {
+            doi = new DOI(value);


It is preferable to use DOI.parse(value) since it adds a bit of cleanup and you don't need to catch the IlegalArgumentException.
https://github.com/JabRef/jabref/blob/master/src/main/java/org/jabref/model/entry/identifier/DOI.java#L97

Good point. Applied.

tobiasdiez · 2019-09-03T09:33:20Z

src/main/java/org/jabref/logic/formatter/bibtexfields/ShortenDOIFormatter.java

+
+        try {
+            doi = new DOI(value);
+            value = shortDOIService.getShortDOI(doi).getDOI();


I like return shortDOIService.getShortDOI(doi).getDOI(); more (since it doesn't override the argument and makes the flow of the method a bit more clear.

Sure. Fixed.

tobiasdiez · 2019-09-03T09:35:42Z

src/main/java/org/jabref/logic/importer/util/ShortDOIService.java

+
+        JSONObject jsonObject = null;
+        try {
+            jsonObject = JsonReader.toJsonObject(urlDownload.asInputStream());


Same here: early return is better

tobiasdiez · 2019-09-03T09:36:57Z

src/main/java/org/jabref/logic/importer/util/ShortDOIServiceResponse.java

+
+import java.util.Objects;
+
+public class ShortDOIServiceResponse {


This class seems to be unused?!

Oh yes... Sorry for that. I changed the approach during work and forgot about it. Removed it.

tobiasdiez · 2019-09-03T09:38:23Z

src/main/java/org/jabref/model/entry/identifier/DOI.java

    // DOI
    private final String doi;
+    // Short DOI
+    private boolean shortDoi;


rename the variable to isShortDoi please

Sure. Renamed.

tobiasdiez · 2019-09-03T09:39:05Z

src/test/java/org/jabref/logic/formatter/bibtexfields/ShortenDOIFormatterTest.java

+@FetcherTest
+class ShortenDOIFormatterTest {
+
+    private ShortenDOIFormatter sut;


we try to avoid abbreviations as much as possible. formatter would be a better name.

Sure. Fixed.

tobiasdiez · 2019-09-03T09:41:18Z

src/test/java/org/jabref/model/entry/identifier/DOITest.java

    }

    @Test
    public void rejectEmbeddedDoi() {
        assertThrows(IllegalArgumentException.class, () -> new DOI("other stuff 10.1006/jmbi.1998.2354 end"));
+        // Short DOI


Please create a new test for the short doi (each test should only contain one assertion - or at least only assert that one specific feature works as intended).

Good call. Extracted all tests related to short DOIs to another tests. I also fixed not handling upper case letters in short DOIs.

dawidowoc · 2019-09-07T16:28:08Z

Thank you for review. I applied changes as you suggested in comments.

Now you have to decide what about this more accessible UI to this feature. IMHO adding it to right-click menu as suggested by @tobiasdiez sounds nice. But as you said it's unfortunately blocked at the moment by #5254. What do you think @tobiasdiez @Siedlerchr @koppor?

Would you like to merge this PR (if you approve it of course) and create a new issue for the UI enhancement in the future or rather do UI in this PR?

Siedlerchr

looks good to be now

Siedlerchr · 2019-09-07T16:48:44Z

I think you can create a new issue for adding it to the context menu and then refer to that blocker issue.

tobiasdiez · 2019-09-08T08:39:45Z

I think we can already merge and I'll add a comment concerning the ui implementation in koppor#343.

Thanks again for your contribution.

30fb68e Create BJEDIS-ABNT-Number (#5255) aafb868 Update geochimica-et-cosmochimica-acta.csl (#5321) 60ba25f british-journal-of-anaesthesia.csl: add comma delimiter between non-sequential citations eg. 1 4 7-9 -> 1, 4, 7-9 (#5313) 67e6564 Reindent/reorder (#5318) c0d2a39 Ruby 3.0.0 (#5309) 76d60ff Update harvard-anglia-ruskin-university.csl (#5310) bc18ac9 Create journal-for-the-study-of-the-new-testament.csl (#5312) aff602c Update journal-of-food-protection.csl (#5315) 4503826 Update muscle-and-nerve.csl (#5317) 3bed58e constant redefinition 4d718a0 update documentaiton link fa99e2f add comma delimiter between succesive numbers d396f8b Allow privileged testing of PRs (#5307) 43b22c7 Update masarykova-univerzita-pravnicka-fakulta.csl, pravnik.csl, iso690-full-note-cs.csl (#5308) 8a31c1e Update copernicus-publications.csl (#5303) 96760bb Update anabases.csl (#5304) 744de6d removed locale (#5300) 7eb0d60 Update aviation-space-and-environmental-medicine.csl (#5297) 2769970 Update ruhr-universitat-bochum-lehrstuhl-fur-industrial-sales-and-ser… (#5298) 51e3f4c Update harvard-university-of-bath.csl (#5299) 5fce84f Create cns-spectrums.csl (#5290) bb8082c Create journal-of-surgical-oncology.csl (#5259) 90c13ae Update ruhr-universitat-bochum-lehrstuhl-fur-industrial-sales-and-ser… (#5288) 4bab1ad Update early-christianity.csl (#5289) 636ba78 Update tatup-zeitschrift-fur-technikfolgenabschatzung-in-theorie-und-… (#5291) b7cc511 Create biotechnologia.csl (#5292) 5bab881 Update journal-of-orthopaedic-trauma.csl (#5287) 5943413 Fix locales (#5285) 302bd65 Update universite-du-quebec-a-montreal-departement-dhistoire.csl (#5286) 860ae48 Add Haaga-Helia University of Applied Sciences Harvard style (#5282) c1c27de Localize Metropolia style title (#5283) 508da89 Fix presentation for Methods of Information in Medicine (#5284) 53e1d0b Create geschichte-und-gesellschaft.csl (#5216) d7ed0cb Create universite-de-geneve-departement-de-francais-moderne.csl (#5212) 80c404b Update journal-of-orthopaedic-trauma.csl (#5281) 20c143a Adding publishers' names (#5280) 6e5cd59 Update sodertorns-hogskola-oxford.csl (#5279) 52f2621 dollar-brace a260294 Create journal-of-microbiology-and-biotechnology.csl (#5277) 1fc979e Create qeios.csl (#5261) 86347b7 GH does this for us -- again, sorry guys b649589 Create experimental-biology-and-medicine.csl (#5276) 12ae0b1 Revert "tell sheldon about the job state" bdcae89 tell sheldon about the job state 1240067 Add Vegetation classification and Survey (#5271) 6f398f0 Major update to Gallia.csl (#5269) 2a74b2c Update filters.yaml (#5273) 20046d2 Update spec_helper.rb (#5272) 2ee0dd8 Create the-sociological-review.csl (#5260) 5b8d09c move filters to inert file to pacify Sheldon (#5268) e5f3315 Localize more language descriptors in style titles (#5270) bfd2942 Localize more language descriptors in style titles (#5267) 35e276f Fix variable used for the label after indication of number of pages (#5240) 60f6371 Create Universidade-do-Estado-do-Rio-de-Janeiro.csl (#5247) d8cc2ae Create the-journal-of-the-acoustical-society-of-america-numeric.csl (#5256) 92259c1 Create journal-of-financial-and-quantitative-analysis.csl (#5264) 6ba8aab Create journal-of-vestibular-research.csl (#5258) 0c88f41 Update european-journal-of-international-law.csl (#5265) cff5abc Put language descriptor within parentheses 4a62709 Update monash-university-harvard.csl (#5253) 64fd1aa Localize more language descriptors in style titles (#5262) f6519cb Localize more language descriptors in style titles (#5257) 170ccae tiny fixes for universitat-basel-iberoromanistik.csl (#5254) b7284c9 Localize more language descriptors in style titles (#5252) f4ef858 Add "Baishideng Publishing Group" dependents (#5251) 266e7c3 Make world-journal-of-hepatology.csl to bpg.csl parent (#5243) 9129098 fix small formatting issues for mclc.csl (#5229) 5d9560b Create crispr-journal.csl (#5249) a217299 Change "Czech" to "Čeština" in titles (#5248) 4fef39a Create journal-of-open-research-software.csl (#5245) 2bff1a6 Change "Dutch" to "Nederlands" in titles (#5242) f28da34 Update spec_helper.rb (#5246) e0e977c Move content from wiki pages to markdown files (#5194) 018304c Update universite-de-montreal-apa.csl (#5239) 3b83e5c Create sodertorns-hogskola-oxford.csl (#5234) 1335378 Stop notifying 8827 port on Zotero servers (#5237) f079b2a Update author-year disambiguation (#5238) 60bb0c9 Update technische-universitat-dresden-medizin.csl (#5236) e374657 Create Leidraad voor juridische auteurs 2019 (Dutch) (#5223) 0450d89 Add new style for U of Mannheim, Germanistische Linguistik (#5228) 81f0689 Create health-sports-rehabilitation-medicine.csl (#5233) c152a44 Update Gemfile.lock (#5235) 748e1eb Update geochimica-et-cosmochimica-acta.csl (#5231) 06b9ce8 Update zeitschrift-fur-theologie-und-philosophie.csl (#5230) e747cb1 haute-ecole-de-gestion-de-geneve: Make polyglot & et al changes 4cfedb7 Create universite-de-sherbrooke-histoire.csl (#5210) a96a61e Update journal-of-glaciology.csl (#5222) c6a94c9 Add Journal of Human Rights (#5227) c5c9c5f Update ruhr-universitat-bochum-lehrstuhl-fur-industrial-sales-and-ser… (#5214) ffb7aa6 Create comparativ.csl (#5215) e07329a Update lancaster-university-harvard.csl (#5220) c075d41 Update mimesis-edizioni.csl (#5219) 502970a Removed space in year only citation (#5218) 13e8c6b Update acta-scientiae-veterinariae.csl (#5209) 0699da6 Remake mammallia.csl for Oct/2020 guidelines. (#5207) b2dd3fd Update journal-of-international-business-studies.csl (#5217) dd52bfe Update quaternaire.csl (#5199) ccb1b0d rebuild webpage and article-journal citations in journal-of-forensic-sciences.csl (#5203) f02f4fb Create pedosphere.csl (#5196) 70dd87a Create open-gender-journal.csl (#5198) d272998 Create the-quarterly-journal-of-economics.csl (#5197) d27cab3 fix locale issues, add cite-locator (#5206) git-subtree-dir: buildres/csl/csl-styles git-subtree-split: 30fb68e

Siedlerchr reviewed Sep 2, 2019

View reviewed changes

Siedlerchr requested changes Sep 2, 2019

View reviewed changes

tobiasdiez requested changes Sep 3, 2019

View reviewed changes

Add shorten DOI field formatter (koppor#343)

cbdd762

dawidowoc changed the title ~~[WIP] Add shorten DOI field formatter~~ Add shorten DOI field formatter Sep 7, 2019

dawidowoc requested review from Siedlerchr and tobiasdiez September 7, 2019 16:28

Siedlerchr approved these changes Sep 7, 2019

View reviewed changes

tobiasdiez merged commit 164843b into JabRef:master Sep 8, 2019

dawidowoc mentioned this pull request Sep 8, 2019

Add Shorten DOI field formatter to Save actions page JabRef/user-documentation#216

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add shorten DOI field formatter #5276

Add shorten DOI field formatter #5276

dawidowoc commented Sep 2, 2019 •

edited

Loading

Siedlerchr Sep 2, 2019 •

edited

Loading

dawidowoc Sep 2, 2019

Siedlerchr left a comment

tobiasdiez left a comment

tobiasdiez Sep 3, 2019

dawidowoc Sep 4, 2019

tobiasdiez Sep 3, 2019

dawidowoc Sep 4, 2019

tobiasdiez Sep 3, 2019

dawidowoc Sep 7, 2019

tobiasdiez Sep 3, 2019

dawidowoc Sep 4, 2019

tobiasdiez Sep 3, 2019

dawidowoc Sep 4, 2019

tobiasdiez Sep 3, 2019

dawidowoc Sep 4, 2019

tobiasdiez Sep 3, 2019

dawidowoc Sep 7, 2019

dawidowoc commented Sep 7, 2019

Siedlerchr left a comment

Siedlerchr commented Sep 7, 2019

tobiasdiez commented Sep 8, 2019


		import java.util.Objects;

		public class ShortDOIServiceResponse {

Add shorten DOI field formatter #5276

Add shorten DOI field formatter #5276

Conversation

dawidowoc commented Sep 2, 2019 • edited Loading

What this PR adds?

Implementation details:

Feedback wanted

Next step

Important

Siedlerchr Sep 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siedlerchr left a comment

Choose a reason for hiding this comment

tobiasdiez 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

dawidowoc commented Sep 7, 2019

Siedlerchr left a comment

Choose a reason for hiding this comment

Siedlerchr commented Sep 7, 2019

tobiasdiez commented Sep 8, 2019

dawidowoc commented Sep 2, 2019 •

edited

Loading

Siedlerchr Sep 2, 2019 •

edited

Loading