-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Story/cite-177 There needs to be an import from Crossref #22
base: develop
Are you sure you want to change the base?
Conversation
…rossrefReferenceImportProcessor accordingly
Can one of the admins verify this patch? |
citesphere-importer/pom.xml
Outdated
@@ -15,7 +15,8 @@ | |||
<spring-data.version>Lovelace-RELEASE</spring-data.version> | |||
<thymeleaf.version>3.0.11.RELEASE</thymeleaf.version> | |||
<spring.kafka.version>2.2.6.RELEASE</spring.kafka.version> | |||
<citesphere.messages.version>0.2</citesphere.messages.version> | |||
<citesphere.messages.version>0.5</citesphere.messages.version> | |||
<crossref-connect-version>0.3-SNAPSHOT</crossref-connect-version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the current version of crossref-connect is 0.2. Where there changes and is there a pull request? If so, it needs to be linked. If not, this needs to be the correct version.
|
||
sendMessage(null, message.getId(), Status.PROCESSING, ResponseCode.P00); | ||
BibEntryIterator bibIterator = null; | ||
bibIterator = new CrossRefIterator(info); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
merge with line 82
|
||
response = response != null ? response : new ItemCreationResponse(); | ||
sendMessage(response, message.getId(), Status.DONE, ResponseCode.S00); // giving error 500 as response code mentioned | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a lot of duplicate code. This method should be in the superclass and in line 83 should getBibIterator()
be called. The rest should then be the same. The logic of how the bibiterator is created will then be in that method.
articles = new ArrayList<>(); | ||
for (String doi : info.getDois()) { | ||
try { | ||
Item item = crossrefService.get(doi); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way it is written right now, this should be done in next
(only one) and also paced to make sure we don't bombard crossref with requests. Even better would probably be to pass in a list of dois and make only one request but that might not be implemented either in our connector or in the crossref api.
chair.setIds(Arrays.asList(contributorID)); | ||
contributors.add(chair); | ||
} | ||
// added Editors & translators to article meta. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the comment.
ref.setReferenceId(itemRef.getKey()); | ||
ref.setReferenceString(itemRef.getUnstructured()); | ||
ref.setReferenceStringRaw(itemRef.getUnstructured()); | ||
references.add(ref); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
content of for loop should go in its own method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this still
return meta; | ||
} | ||
|
||
public List<Contributor> mapPersonToContributor(List<Person> personList) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this need to be public?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to private
@@ -41,8 +42,11 @@ public ObjectNode generateJson(JsonNode template, BibEntry entry) { | |||
ItemJsonGenerator generator = generators.get(entry.getArticleType()); | |||
if (generator != null) { | |||
return generator.generate(template, entry); | |||
} else if (entry instanceof Publication){ | |||
generator = generators.get("CrossRef"); | |||
return generator.generate(template, entry); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this code for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed this block, as crossref generator is removed.
} | ||
|
||
@Test | ||
public void testStartImport_successful() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test naming conventions not followed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleted the tests as CrossrefRefernceImport does not have much now.
@Test | ||
public void testStartImport_successful() throws Exception { | ||
when(info.getDois()).thenReturn(Arrays.asList("10.1234/example1", "10.5678/example2")); | ||
when(handlerRegistry.handleFile(info, null)).thenReturn(bibIterator); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the crossref importer doesn't have a handlerregistry, does it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleted the tests as CrossrefRefernceImport does not have much now.
no need to make comments like "done" or "removed", etc. I just get more notifications and it drowns out the comments I really need to read and respond to. |
} | ||
return file; | ||
} | ||
protected abstract BibEntryIterator getbibIterator(KafkaJobMessage message, JobInfo info); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be getBibEntryIterator
public final static String EDITED_BOOK = "edited-book"; | ||
public final static String PROCEEDINGS_ARTICLE = "proceedings-article"; | ||
public final static String DISSERTATION = "dissertation"; | ||
public final static String BOOK_SECTION = "book-section"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this already exists (BOOK_CHAPTER
)
@@ -12,6 +12,34 @@ public class Publication implements BibEntry { | |||
public final static String NEWS_ITEM = "newspaperArticle"; | |||
public final static String PROCEEDINGS_PAPER = "conferencePaper"; | |||
public final static String DOCUMENT = "document"; | |||
// publication types in CrossRef | |||
public final static String MONOGRAPH = "monograph"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can use BOOK
instead
public final static String POSTED_CONTENT = "posted-content"; | ||
public final static String COMPONENT = "component"; | ||
public final static String EDITED_BOOK = "edited-book"; | ||
public final static String PROCEEDINGS_ARTICLE = "proceedings-article"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the same as PROCEEDINGS_PAPER
I believe
@@ -81,45 +65,105 @@ public void init() { | |||
itemTypeMapping.put(Publication.NEWS_ITEM, ItemType.NEWSPAPER_ARTICLE); | |||
itemTypeMapping.put(Publication.PROCEEDINGS_PAPER, ItemType.CONFERENCE_PAPER); | |||
itemTypeMapping.put(Publication.DOCUMENT, ItemType.DOCUMENT); | |||
itemTypeMapping.put(Publication.MONOGRAPH, ItemType.BOOK); | |||
itemTypeMapping.put(Publication.JOURNAL_ISSUE, ItemType.JOURNAL_ARTICLE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is wrong I believe, an issue is not an article. I think we will simply not deal with those and not import issues.
meta.setIssns(issnList); | ||
|
||
List<Contributor> contributors = new ArrayList<>(); | ||
if(item.getChair() != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
|
||
private ArticleMeta parseArticleMeta(Item item) { | ||
ArticleMeta meta = new ArticleMeta(); | ||
meta.setArticleTitle(String.join(", ", item.getTitle())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
ref.setReferenceId(itemRef.getKey()); | ||
ref.setReferenceString(itemRef.getUnstructured()); | ||
ref.setReferenceStringRaw(itemRef.getUnstructured()); | ||
references.add(ref); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
BibEntry nextEntry = new Publication();; | ||
|
||
try { | ||
Item item = crossrefService.get(info.getDois().get(currentIndex)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of keeping an int around, just get an interator from the dois list and keep that
@@ -13,6 +13,7 @@ | |||
import com.fasterxml.jackson.databind.node.ObjectNode; | |||
|
|||
import edu.asu.diging.citesphere.importer.core.model.BibEntry; | |||
import edu.asu.diging.citesphere.importer.core.model.impl.Publication; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the abstract import processor should still stay in the impl
package, it's part implementation after all.
|
||
private ArticleMeta parseArticleMeta(Item item) { | ||
ArticleMeta meta = new ArticleMeta(); | ||
meta.setArticleTitle(String.join(", ", item.getTitle())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this still
ref.setReferenceId(itemRef.getKey()); | ||
ref.setReferenceString(itemRef.getUnstructured()); | ||
ref.setReferenceStringRaw(itemRef.getUnstructured()); | ||
references.add(ref); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this still
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unaddressed comments
Looks fine. Once the Citesphere PR is ready to be tested, I'll deploy this. |
Guidelines for Pull Requests
If you haven't yet read our code review guidelines, please do so, You can find them here.
Please confirm the following by adding an x for each item (turn
[ ]
into[x]
).Please provide a brief description of your ticket
The user should be able to search in crossref to references to import. whatever search results from Crossref a user selects should then be imported into citesphere. This could be all search results or just a few or one.
For now, let's put add an entry to the import menu and let the user select which group to import the results into from a dropdown menu.
The importing should be done from Citesphere Importer (not Citesphere), similar how entries from files are imported.
For querying crossref, this library should be used: https://github.com/diging/crossref-connect. However, this is not yet released into maven central, so talk to Julia before picking up this ticket.
... Put ticket description here and add link to ticket ...
https://diging.atlassian.net/browse/CITE-177
Are there any other pull requests that this one depends on?
diging/citesphere-messages#12
diging/citesphere#271
Anything else the reviewer needs to know?
... describe here ...