Story/q20 19 #23

Pooja444 · 2021-12-14T23:27:58Z

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]).

I have removed all code style changes that are not necessary (e.g. changing blanks across the whole file that don’t need to be changed, adding empty lines in parts other than your own code)
I am not making any changes to files that don’t have any effect (e.g. imports added that don’t need to be added)
I do not have any sysout statements in my code or commented out code that isn’t needed anymore
I am not reformatting any files in the wrong format or without cause.
I am not changing file encoding or line endings to something else than UTF-8, LF
My pull request does not show an insane amount of files being changed although my ticket only requires a few files being changed
I have added Javadoc/documentation where appropriate
I have added test cases where appropriate
I have explained any part of my code/implementation decisions that is not be self-explanatory

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

…tests

….0 into story/Q20-18

jdamerow

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.

jdamerow · 2023-05-24T17:06:37Z

quadriga/pom.xml

-
+
+        <conceptpower.base.url></conceptpower.base.url>
+        <conceptpower.id.endpoint></conceptpower.id.endpoint>


remove it here

jdamerow · 2023-05-24T17:08:00Z

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

jdamerow

It really throws me off that the ConceptPower classes capitalize the "P" 😅 the app is called "Conceptpower". Maybe change the class names? :)

jdamerow · 2024-11-07T15:15:28Z

quadriga/src/main/java/edu/asu/diging/quadriga/core/conceptpower/model/ConceptCache.java

+        // If both old and new cache values are null/blank/empty, nothing has changed
+        // If old value is null/blank/empty, new value is not null/blank/empty, difference present
+        // If old value is not null/blank/empty, new value is null/blank/empty, difference present
+        // If both are not null/blank/empty, we need to check difference


this should probably go into the javadoc (which is missing)

jdamerow · 2024-11-07T15:17:31Z

quadriga/src/main/java/edu/asu/diging/quadriga/core/conceptpower/model/ConceptCache.java

+        if (isDifferentString(conceptCache.getPos(), this.getPos())) return -1;
+        if (isDifferentString(conceptCache.getTypeId(), this.getTypeId())) return -1;
+        if (isDifferentString(conceptCache.getUri(), this.getUri())) return -1;
+        if (isDifferentString(conceptCache.getWord(), this.getWord())) return -1;


this is not following our formatting guidelines. or it could be put as an OR statement (formatted one condition a line), so that only once -1 is returned.

jdamerow · 2024-11-07T15:17:48Z

quadriga/src/main/java/edu/asu/diging/quadriga/core/conceptpower/model/ConceptType.java

+        if(isDifferentString(type.getDescription(), this.getDescription())) return -1;
+        if(isDifferentString(type.getName(), this.getName())) return -1;
+        if(isDifferentString(type.getUri(), this.getUri())) return -1;
+        return 0;


same as before

jdamerow · 2024-11-07T15:21:30Z

...edu/asu/diging/quadriga/core/conceptpower/service/impl/ConceptPowerConnectorServiceImpl.java

+    @Override
+    public ConceptPowerReply getConceptPowerReply(String conceptURI) {
+        Map<String, String> pathVariables = new HashMap<>();
+        pathVariables.put("concept_uri", conceptURI);


these are not path variables, are they? but parameters?

jdamerow · 2024-11-07T15:24:42Z

...edu/asu/diging/quadriga/core/conceptpower/service/impl/ConceptPowerConnectorServiceImpl.java

+            logger.error("Could not get concept for URI: " + conceptURI + " at URL: " + conceptPowerURL, e);
+        }
+
+        return null;


it looks like don't where you use the return value, you don't check for null, which means it'll throw a nullpointer if conceptpower is down? or throws an error? The better approach would probably be throw an exception if there is no response or an exception from conceptpower, and then handle it appropriate where it makes sense.

jdamerow · 2024-11-07T15:30:14Z

...ain/java/edu/asu/diging/quadriga/core/conceptpower/service/impl/ConceptPowerServiceImpl.java

+    private boolean updateConceptCache(ConceptCache conceptCache, ConceptCache conceptCacheOld, String uri) {
+
+        // ConceptPower returned a concept and either no conceptCache entry exists in the DB
+        // or if one exists, it is different from the current conceptCache entry


this should probably be part of the javadoc

jdamerow · 2024-11-07T15:32:04Z

...ain/java/edu/asu/diging/quadriga/core/conceptpower/service/impl/ConceptPowerServiceImpl.java

+        List<ConceptEntry> conceptEntries = conceptPowerReply.getConceptEntries();
+        ConceptCache conceptCache = null;
+
+        if (conceptEntries != null && !conceptEntries.isEmpty()) {


turn this if around and just return if this condition is not met. It'll unindent all the following code.

jdamerow · 2024-11-07T15:33:28Z

...ga/src/main/java/edu/asu/diging/quadriga/core/service/impl/MappedTripleGroupServiceImpl.java

@@ -145,7 +151,13 @@ public MappedTripleGroup get(String collectionId, MappedTripleType mappedTripleT
        // we create a new MappedTripleGroup entry
        if (mappedTripleGroup == null) {
            mappedTripleGroup = add(collectionId, mappedTripleType);
+
+            // If mappedTripleGroup add fails


shouldn't this be made known to the upper layers via an exception?

jdamerow · 2024-11-07T15:39:24Z

quadriga/src/main/resources/config.properties

@@ -6,6 +6,9 @@ db.password=${db.password}
 neo4j.url=${neo4j.url}
 neo4j.database=${neo4j.database}

+conceptpower_base_url=${conceptpower.base.url}


there is no conceptpower.base.url property in the pom, is there? If this property is required here, it needs to be added to the pom.

jdamerow · 2024-11-07T15:39:43Z

quadriga/src/main/resources/config.properties

 citesphere_client_id=${citesphere.client.id}
-citesphere_client_secret=${citesphere.client.secret}
+citesphere_client_secret=${citesphere.client.id}


this does not look right

Pooja444 and others added 30 commits August 31, 2021 15:43

[story/Q20-11] completed edit collections implementation, added unit …

03e7120

…tests

[story/Q20-11] addressed review comments for edit collections

e4a5a8b

[story/Q20-11] addressed review comments

0a74117

[story/Q20-12] added collectionId param while submitting network

2ada5f9

[story/Q20-12] added collectionId to EventGraph

19f9069

[story/Q20-3] added token validity check

ba4267b

[Q20-7] Adding Citephere config

2167f2a

[story/Q20-3] added token validation logic

bd62381

[Q20-7] - Fetch apps from citesphere after registering quadriga

3d8be96

[story/Q20-3] completed check token dev, started unit tests

83e9aa8

[Q20-7] - Working on front-end for multiple app selection

df99e9e

[story/Q20-3] partially completed unit tests

a23bc9a

[story/Q20-3] added few more test cases

d31e504

[story/Q20-3] completed all unit tests

b1c48b8

[story/Q20-3] added required=true for authHeader

4b261e3

[Q20-7] - Replaced pop-up selection with list selection

a2a3ccf

[story/Q20-12] changed Objects.isNull to simple null check

5ead2a8

[story/Q20-3] changed Objects.isNull to simple null check

5e63e67

[Q20-7] - Completed front-end for add collections part

e6db79d

[Q20-7] - fixed test case

ecab67d

[story/Q20-18] added view to display collection info

d3cbe2a

[story/Q20-18] added creationTime to network graph

dc4c9c3

[story/Q20-18] added last network submission info

4ddadf0

[story/Q20-12] addressed review comments

27ed3e7

[story/Q20-12] resolved merge conflicts with story Q20-11

ad11972

[story/Q20-12] removed double test cases

82122dc

[story/Q20-12] caught IllegalArgumentException while finding collection

1425d51

[story/Q20-12] removed unnecessary null check

a6375e7

[story/Q20-18] Merge branch 'develop' of github.com:diging/quadriga-2…

f2f3873

….0 into story/Q20-18

[story/Q20-12] removed unncessary file changes

2035910

[Q20-19] Addressed review comments.

4f77dc2

ShivaniYerram reopened this May 23, 2023

jdamerow requested changes May 24, 2023

View reviewed changes

jdamerow closed this May 24, 2023

pooja-thalur added 2 commits September 17, 2024 15:24

[Q20-19] review comments addressed

2db6fc6

pooja-thalur reopened this Sep 20, 2024

pooja-thalur closed this Sep 20, 2024

pooja-thalur added 8 commits September 24, 2024 08:50

[Q20-19] fixing codefactor issue

7519a39

[Q20-19] codefactor issue

2527a8b

[Q20-19] indentation changes

6fe474e

[Q20-19] upgrading security core dependency

7edb536

[Q20-19] dependency upgrade

09d697f

[Q20-19] jackson dependency upgraded

4228f31

[Q20-19] reverting dependency update changes

c18e489

[Q20-19] unnecessary change removed

5cbc2a1

pooja-thalur reopened this Sep 25, 2024

jdamerow requested changes Nov 7, 2024

View reviewed changes

jdamerow closed this Nov 7, 2024

pooja-thalur added 4 commits December 17, 2024 16:36

[Q20-19] resolving review comments

54f4fd2

[Q20-19] addressing review comments

76f159e

[Q20-19] review comments addressed

718301c

[Q20-19] fixed testcases

c361808

pooja-thalur reopened this Dec 20, 2024

pooja-thalur added 2 commits December 20, 2024 09:31

[Q20-19] codefactor issues resolved

2f7c527

[Q20-19] codefactor issues fixed

2e028dd

pooja-thalur closed this Dec 20, 2024

[Q20-19] reverted the config change

996f7d7

pooja-thalur reopened this Jan 2, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Story/q20 19 #23

Story/q20 19 #23

Pooja444 commented Dec 14, 2021 •

edited by pooja-thalur

Loading

jdamerow left a comment

jdamerow May 24, 2023

jdamerow commented May 24, 2023

jdamerow left a comment

jdamerow Nov 7, 2024

jdamerow Nov 7, 2024

jdamerow Nov 7, 2024

jdamerow Nov 7, 2024

jdamerow Nov 7, 2024

jdamerow Nov 7, 2024

jdamerow Nov 7, 2024

jdamerow Nov 7, 2024

jdamerow Nov 7, 2024

jdamerow Nov 7, 2024



		<conceptpower.base.url></conceptpower.base.url>
		<conceptpower.id.endpoint></conceptpower.id.endpoint>

Story/q20 19 #23

Are you sure you want to change the base?

Story/q20 19 #23

Conversation

Pooja444 commented Dec 14, 2021 • edited by pooja-thalur Loading

Guidelines for Pull Requests

Please provide a brief description of your ticket

Anything else the reviewer needs to know?

jdamerow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdamerow commented May 24, 2023

jdamerow 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

Pooja444 commented Dec 14, 2021 •

edited by pooja-thalur

Loading