-
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/q20 19 #23
base: develop
Are you sure you want to change the base?
Story/q20 19 #23
Conversation
….0 into story/Q20-18
|
||
/** | ||
* | ||
*/ |
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.
remove if there is no javadoc
/** | ||
* | ||
*/ | ||
private static final long serialVersionUID = 6260562492095202271L; |
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 start at 1
/** | ||
* | ||
*/ | ||
private static final long serialVersionUID = 4228339422371096758L; |
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.
same as before
quadriga/pom.xml
Outdated
@@ -44,6 +44,9 @@ | |||
<citesphere.base.url></citesphere.base.url> | |||
<citesphere.client.id></citesphere.client.id> | |||
<citesphere.client.secret></citesphere.client.secret> | |||
|
|||
<conceptpower.base.url></conceptpower.base.url> | |||
<conceptpower.id.url></conceptpower.id.url> |
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 used?
private String conceptPowerBaseURL; | ||
|
||
@Value("${conceptpower_id_url}") | ||
private String conceptPowerIdURL; |
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.
that's the endpoint not the url, right?
|
||
if (size != null) { | ||
sizeLimit = size; | ||
sizeLimit = sizeLimit < 1 ? 10 : sizeLimit; |
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.
why not changing size
? is a new variable necessary?
SUBJECT(1), | ||
OBJECT(1); | ||
|
||
private int groupId; |
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.
? neesd javadoc
@Autowired | ||
private ConceptPowerService conceptPowerService; | ||
|
||
private Logger logger = LoggerFactory.getLogger(getClass()); |
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.
shoudl be first
citesphere_client_secret=${citesphere.client.secret} | ||
citesphere_base_url=${citesphere.base.url} | ||
conceptpower_base_url=${conceptpower.base.url} | ||
conceptpower_id_url=/rest/Concept?id={concept_uri} |
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.
that's the endpoint not the url
conceptpower_id_url=/rest/Concept?id={concept_uri} | ||
|
||
citesphere_client_id=OAUTHCLIENT32 | ||
citesphere_client_secret=03c2b326-88ba-4a38-866a-fc6f35ed0423 |
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.
these should not be in here (secret information!) and also should be defined in the pom.
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 regenerated the secret since this is compromised now.
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 unaddressed comments.
quadriga/pom.xml
Outdated
|
||
|
||
<conceptpower.base.url></conceptpower.base.url> | ||
<conceptpower.id.endpoint></conceptpower.id.endpoint> |
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 that used anywhere?
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.
@jdamerow , It's value is currently defined in config.properties. Shall I remove this entry from pom.xml or give the value 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.
remove it 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.
there is a lot of code here that's also in the other PR I just reviewed, isn't there? Please reopen once the common code has been merged.
quadriga/pom.xml
Outdated
|
||
|
||
<conceptpower.base.url></conceptpower.base.url> | ||
<conceptpower.id.endpoint></conceptpower.id.endpoint> |
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.
remove it here
also, there are conflicts. |
# Conflicts: # quadriga/pom.xml # quadriga/src/main/java/edu/asu/diging/quadriga/core/model/Collection.java # quadriga/src/main/java/edu/asu/diging/quadriga/core/service/CollectionManager.java # quadriga/src/main/java/edu/asu/diging/quadriga/core/service/impl/EventGraphServiceImpl.java # quadriga/src/main/java/edu/asu/diging/quadriga/core/service/impl/MappedTripleGroupServiceImpl.java # quadriga/src/main/java/edu/asu/diging/quadriga/web/DisplayCollectionController.java
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
(you can copy the ticket if it hasn't changed)
https://diging.atlassian.net/browse/Q20-19
On the collection page, it should be possible to browse all networks in collection -
basically a paginated list of all networks ordered by submission date. When the user clicks on a network it would be visualized (similar to the old quadriga).
Anything else the reviewer needs to know?
This story is dependent on story Q20-3, Q20-7, Q20-12, Q20-18 and since all of these stories are yet to be merged, this PR may show a lot of file changes that are actually part of the above stories. This will go away once all of these are merged