-
Notifications
You must be signed in to change notification settings - Fork 3
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 #271
base: develop
Are you sure you want to change the base?
Conversation
Can one of the admins verify this patch? |
citesphere/pom.xml
Outdated
<citesphere.model.version>1.19</citesphere.model.version> | ||
<crossref-connect-version>0.1</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.
is there a reason citesphere uses a different version than the importer?
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 version to 0.3-SNAPSHOT, which is latest version in both repositories.
Map<String, Object> response = new HashMap<>(); | ||
try { | ||
jobManager.createJob((IUser) authentication.getPrincipal(), groupId, dois); | ||
System.out.println("============================================================================================================="); |
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.
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.
please follow our testing naming convention
citesphere/pom.xml
Outdated
<citesphere.model.version>1.19</citesphere.model.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.
there is not 0.3-SNAPSHOT version. if you made changes to this plugin, there needs to be a pr (and that needs to be linked here).
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 it to 0.2 as citesphere-import
</div> | ||
<div id="failAlert" style="display: none;" class="alert alert-danger alert-dismissible fade in" role="alert"> | ||
<button type="button" class="close" data-dismiss="alert" aria-label="Close"><span aria-hidden="true">x</span></button> | ||
Import Failed! Please check the logs |
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 are no logs the user could check, so this message is confusing.
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 the message to actual error received from controller.
assertNotNull(job); | ||
assertEquals(JobStatus.FAILURE, job.getStatus()); | ||
assertEquals(1, job.getPhases().size()); | ||
assertEquals("Error creating message", job.getPhases().get(0).getMessage()); |
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 not worth checking; if the text changes the method logic won't change but it'll make the test fail.
ArrayNode doisNode = mapper.createArrayNode(); | ||
((IImportCrossrefJob)job).getDois().forEach(d -> doisNode.add(d)); | ||
node.set("dois", doisNode); | ||
node.put("groupId", ((IImportCrossrefJob)job).getCitationGroup()); |
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 it's time to clean this up. let's create a map that maps object type to methods that are then called here.
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
$("#failAlert").show(); | ||
} | ||
} | ||
}); |
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.
something is off with the indentation
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.
?
ArrayNode doisNode = mapper.createArrayNode(); | ||
((IImportCrossrefJob)job).getDois().forEach(d -> doisNode.add(d)); | ||
node.set("dois", doisNode); | ||
node.put("groupId", ((IImportCrossrefJob)job).getCitationGroup()); |
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
$("#failAlert").show(); | ||
} | ||
} | ||
}); |
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.
?
…into story/CITE-177
List<String> doisList = new ArrayList<>(); | ||
((IImportCrossrefJob)job).getDois().forEach(d -> doisList.add(d)); | ||
reponse.put("dois", doisList); | ||
reponse.put("groupId", ((IImportCrossrefJob)job).getCitationGroup()); |
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.
Not sure where the comment went, but I made one here before about handling this differently (put the code in methods and map those to job types).
} else if (job instanceof IExportJob) { | ||
handleExportJob(response, (IExportJob) job); | ||
} else if (job instanceof IImportCrossrefJob) { | ||
handleImportCrossrefJob(response, (IImportCrossrefJob) job); |
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 it's time to clean this up. let's create a map that maps object type to methods that are then called here.
Create a map and map class to method, then call map.get(job.getClass()) to get method to execute.
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-importer#22
diging/citesphere-messages#12
Anything else the reviewer needs to know?
... describe here ...